I hate buggy software. However, as a developer, I know how difficult it is to write bug-free software and so I am always looking for new ways to learn how to write better software. One of those ways is exception based programming. Sadly, exceptions are often glossed over in samples and books so exception anti-patterns tend to propagate.
Take a look at the following code…
public void HorrifyingMethod()
{
try
{
Cursor preCursor = Cursor.Current;
Cursor.Current = Cursors.WaitCursor;
DoSomething();
DoSomethingElse();
try
{
Log.Write("I did something");
}
catch {}
Cursor.Current = preCursor;
}
catch (Exception ex)
{
if (ex.Message == "Failed")
throw ex;
else
Log("Something failed", ex);
}
}
There’s so much wrong with this method it makes my skin crawl. Let’s start with the obvious:
Catching the base exception type
There aren’t a lot of cases where this is ever justified. When you catch an exception, you are effectively stating that you understand the nature of the failure, and you are going to resolve the problem (logging, by the way, is not resolving the problem). Our nested try/catch block may as well say “if the server caught fire, I’m just going to ignore it.” When you write an exception method, it is helpful to say to yourself, “The managed runtime exploded, therefore I am …” and say what your catch block does. If you find yourself saying something like “The managed runtime exploded, therefore I am returning the default value” you can see how problematic this really is.
Discriminating on exception data rather than exception type
Exceptions should describe the nature of the problem, not where it came from. Every exception already comes with a stack trace so we know where it came from. A good exception is something like “TimeoutException” rather than “MailComponentException” If you find yourself commonly digging through exceptions to determine what exactly went wrong, you are using a poorly designed library. If you are throwing exceptions, use an existing exception class if it fits the problem, or write a new class that describes the problem. The exception type itself is the filter used for catching, so it’s important for exceptions to describe the nature of the failure.
Re-throwing a caught exception from a new catch block
There are times when you might catch an exception, and after doing some programmatic investigation decide that you can’t actually handle it and you need to rethrow it. Never rethrow the same instance that you caught in the catch or you’ll wipe out your stack. The correct way to rethrow a caught exception is just a single “throw;” statement with no variable.
public void CorrectRethrow()
{
try
{
SomeMethod();
}
catch(Exception ex)
{
if (!ex.SomeProperty)
throw;
}
}
Eating the exception
This is probably by far one of the worst offenders. Exceptions work well because they are an opt-out method of detecting abnormalities rather than past “opt-in” methods such as errorcodes. Eating exceptions is rarely correct. If you are developing a library for use by other developers, you should not be making decisions for them with regard to exceptions. Always throw the exception to the library user so that she can handle it as she sees fit. The HorrifyingMethod() code shows two exception eating problems: The inner try/catch block is catching both managed and unmanaged exceptions, and completely hiding the fact that anything went wrong. This is all too common, and contributes to bugs, failures, and strange side-effects with nearly impossible to trace causes. The outer block logs the exception, making a token gesture of “handling” it. Imagine this method being called from a button click event. The user keep smashing the button, the program continues to fail to execute the procedure, and somewhere an obscure log file is recording all the detail. Logging an exception is not handling it.
(Note that if you do have more than one try/catch/finally in a method, your design is probably screaming for an ExtractMethod refactoring. Your method is almost certainly too large if there is a need for more than one)
Lack of a finally block to ensure consistency of changes in the method
There should be far more try/finally blocks in your code than try/catch blocks. In order to write exception-safe code, anything in a try block must be reverted by a finally block to leave the application in a consistent state. When an exception was thrown in Visio, it would bring up a dialog box saying that the application state was inconsistent and advised you to restart the application. This is decidedly not okay in the .NET world.