When reviewing code, I often encounter problems in the way that exceptions are handled. I’m not going to cover guidelines for designing your own exception hierarchy or whether you should derive from ApplicationException or Exception for custom exceptions. (You should derive from Exception, if you must know. There is really no reason for ApplicationException and was a design goof by the BCL Team, as noted in .NET Framework Standard Library Annotated Reference Vol1.) Here’s five code samples showing improper exception handling and how to fix it. We’ll start from the most severe and go to the more cosmetic…
Code Sample #1:
try {
// code
} catch (Exception e) {
// better do something here
}
Whoah!!! We’re losing the exception completely. This is also known as swallowing the exception. This is really bad. If an exception is ever thrown, we’ll never hear about it. Even with the best of intentions, it’s too easy to forget to go back and fill in these types of placeholders. If you want to note that an exception handler needs to be implemented, I would recommend the following code instead:
try {
// code
} catch (Exception e) {
// TODO: better do something here
throw;
}
The TODO will highlight this code in the TODO list in Visual Studio. The throw repropagates the exception so we don’t lose it. We can remove the throw later if we actually handle with the exception.
Code Sample #2:
try {
// code
} catch (ApplicationException e) {
if(e.Message.StartsWith(“Some custom error message thrown by another piece of code”)) {
// Handle the exception
} else {
throw;
}
}
YIKES!!! This code is wrong for so many reasons. Simplest is that your error handling code will fail if you correct (or introduce) a typo in your other code’s error message and you don’t get any compile-time checking. It also breaks horribly as soon as you internationalize your error messages. If you need to handle a particular type of exception, derive a new exception type. If you’re using Visual Studio 2005, there is even a code snippet for new exception types. Simply create a new file, delete the class definition, and type “exception-TAB-TAB”. Presto new exception type. You can then explicitly catch that exception as follows:
try {
// code
} catch (CustomException e) {
// Handle the exception
}
Code Sample #3:
try {
// code
} catch (Exception e) {
throw new ApplicationException(“Some useful message about what we’re doing: ” + e.Message);
}
Whoah again. We just lost a lot of information about the original exception, including its stack trace and any inner exceptions it contained and made our debugging lives a lot harder. I would recommend the following code instead:
try {
// code
} catch (Exception e) {
throw new ApplicationException(“Some useful message about what we’re doing”, e);
}
This code propagates the original exception as the innerException so that we can see the exact origin of the failure.
Code Sample #4:
try {
// code
} catch (Exception e) {
// do something like logging
throw e;
}
This naively looks like reasonable code to repropagate an exception, but it contains a subtle flaw – you’re killing the stack trace. The correct code is:
try {
// code
} catch (Exception e) {
// do something like logging
throw;
}
A simple throw causes the current exception to continue to propagate including stack trace. You have to watch that your logging or other operations don’t throw an exception that can kill the current exception.
Code Sample #5:
try {
// code
} catch (Exception e) {
// Execute code, but never use e, which causes a “variable, e, never used” warning
}
This is minor as it only produces a warning, but if you’re not going to use the variable, don’t declare it! The following code has exactly the same behaviour, but does not produce the compiler warning resulting in easier to read build output.
try {
// code
} catch (Exception) { // We don’t need to refer to Exception, so don’t give it a name
// Execute code
}
or alternatively:
try {
// code
} catch { // We don’t need to refer to Exception, so don’t give it a name
// Execute code
}
This is just a sampling of common problems with exception handling code that I’ve encountered during code reviews. Please feel free to add any additional ones in the comments. Happy exception handling!
