(this is a really long post...only read it if you (a) don't know what try/catch is OR (b) actually write catch(Exception ex) or catch{ })
The first thing I look for when evaluating someone's code is a try/catch block. While it isn't a perfect indicator, exception handling is one of the few things that quickly speak about the quality of code. Within seconds you might discover that the code author doesn't have a clue what he or she is doing. It may be fun to point and laugh at, but poor exception handling can have a serious impact on the maintainability of a system. Proper exception handling isn't very difficult, technically there's nothing to it. It all comes down to grasping the fundamentals. Too many developers consider exceptions dangerous to the health of a system. If you come from an exception-less language, you might be cursing Microsoft developers for ever having introduced them in the framework.
All exceptions in the .NET framework inherit from the base
System.Exception class. This class exposes a handful of members you are likely familiar with, such as
StackTrace and
Message. Exceptions are raised by the framework, a library you are using or your own code when an exceptional situation occurs. While I don't often run into over zealous use of exceptions, it is important to understand that they are meant to be used for truly exceptional scenarios. For example, if a user's registration fails because the email is already in use, throwing an exception might not be appropriate. You'll often hear people say "only use exceptions in exceptional cases", as I just did. This is very sound advice when throwing your own exception, but what about catching and handling exceptions? Well the fundamental point to understand is the same, when an exception does get thrown, be aware that something truly exceptional happened. That might sound a little bit obvious, but a common example will show just how poorly understood the point is:
try
connection.Open()
command.ExecuteNonQuery()
catch
return false
end tryYou might have seen, or even written, similar code before. In
a previous blog post, I pointed out almost identical code in official Macromedia documentation. There's actually more than 1 problem in the above code, but let's focus on the catch block - the place the exception is supposed to get
handled. Does anyone see the exception being handled? It isn't. Instead, we've been warned that something truly bad has happened, and swept it under the rug. This is known as
exception swallowing, and it's a sure sign of someone who's afraid of and doesn't understand exceptions. If you truly understand that an exception is a sign of an exception situation, you'll never dare sweep it under the rug!
If you aren't supposed to swallow them, what's the game plan going to be? The first thing to realize is that much more often than not, there's nothing you'll actually be able to do with exceptions. If I was to guess as to why developers get hung up on exceptions, I'd have to say this is it. For many developers, having an exception bubble up through code, where it'll eventually cause the application to crash, seems unimaginable. For some reason, these same developers don't seem to be too worried about not being able to connect to their database - we'll just return false.
The simple truth is that if you can't actually handle the exception, don't. We'll implement logging and friendly error messages much higher up in the code, where it can be globally reused and easily maintained. If the above code makes it to a production server, no error message will be displayed, no meaningful data will be recorded and it might take hours (or days) before you even know something's wrong.
Rethrownig exceptionsDoes that mean that you should never catch exceptions? Not necessarily, but you need to be careful what you do in there. One of the few technical hangups developers have is how to rethrow a caught exception. Since it might not be obvious at first why you'd do that, let's look at an example. Pretend we've build code that allows user's to register. Our code will first enter the record into the database (assuming all business rules pass), and then send a confirmation email out that details how the account can be activated. If sending out the email fails, we'll want to remove the record from the database so that the user can try again with the same username/email (else he or she will get an error saying the username or email is already in use). With proper use of exceptions, this is a simple matter to accomplish:
try
{
Emailer.SendNewUserActivation(this)
}
catch (SmtpException ex)
{
this.Delete();
throw;
}In the above code we haven't handle the exception, but we've done some important cleaning up. The code can't go in the finally clause because that's called on failure AND success (and we don't want to delete the record if everything was a success). The real gem in the code is the throw statement. This keeps the exception on the stack, where it'll bubble upwards. The calling code won't know that we ever caught the exception and did a bit of internal house-cleaning - which is just what we want because we did nothing to handle the exception.
(in the comments below, Mark Kamoski has some alternative solutions to the code above (along with some concerns) that I think are worthwhile, so scroll down and find his post!)
There are two variations to the code above. Instead of simply using throw; we could actually use
throw ex;. The difference between the two, though subtle, it quite important to understand. When you use
throw ex; you'll modify the call stack and make it look like your code was the source of the exception. You are effectively throwing away useful debugging information. As I've said before, when you simply throw; it's as though you didn't do anything at all! We'll talk more about throwing your own exceptions in a later post, but if you want to actually be involved in the exception bubbling chain, it's generally better to throw a new exception and specified the caught exception as the inner exception, such as:
throw new SomeTypeOfException("User Activation send failed", ex);Cleaning up after yourselfIf you follow my advice (and I'll give you a much better source than just my own word soon), you'll end up with very few Try/Catch statements. Instead what you should be using are Try/Finally statements, or the using statement. While you might not be able to handle an exception, you can certainly clean up after yourself. Most people know this, but finally happens whether an exception is raised or not, and is pretty much guaranteed to happen - so you can return in your try block and rest easy knowing your finally cleaned up your resources. Here's an example:
dim connection as new SqlConnection(GET_CONNECTION_STRING_FROM_CONFIG)
dim command as new SqlCommand(connection)
'...set stuff up
try
connection.Open()
'might wanna check for DbNull or something
return cint(command.ExecuteScalar)
finally
connection.Dipose()
command.Dispose()
end tryIf you instantiate your code inside the try, make sure to check for nothing before disposing:
dim connection as SqlConnection
dim command as SqlCommand
try
connection = new SqlConnection(GET_CONNECTION_STRING_FROM_CONFIG)
command = new SqlCommand(conection)
'...set stuff up
connection.Open()
'might wanna check for DbNull or something
return cint(command.ExecuteScalar)
finally
if (not connection is nothing) then
connection.Dipose()
end if
if (not command is nothing) then
command.Dispose()
end if
end try The above code is a little messy, which is exactly why C# and VB.NET (as of 2005) support the using keyword:
using (SqlConnection connection = new SqlConnection(...))
{
using (SqlCommand command = new SqlCommand(...))
{
//..set stuff up
connection.Open();
return Int32.Parse(command.ExecuteScalar);
}
}I've heard some people say they dislike nested using statements, but if you compare the two examples, I think it's much cleaner, readable and less error prone.
More on actually catching exceptionsNow there are some times when you'll be able to handle an exception. Everyone knows that you always catch specific exceptions first and work your way towards the base exceptions. You should also, never-ever catch
System.Exception. Say we wrote code that took a query string value (or any string for that matter) and tried to turn it into an integer. If it fails, we want to default to a given value. You might write something like:
public static int ParseInt(string value, int defaultValue)
{
try
{
return Int32.Parse(stringValue);
}
catch(Exception ex)
{
return defaultValue;
}
}That's wrong. What happens if Int32.Parse threw a
ThreadAbortException or an
OutOfMemoryException, does returning a defaultValue handle those exceptions? No. Instead, you need to catch only the specific exceptions you are handling:
try
{
return Int32.Parse(stringValue);
}
catch (FormatException ex)
{
return defaultValue;
}
catch (InvalidCastException ex)
{
return defaultValue;
}
catch (OverflowException ex)
{
return defaultValue;
}Of course, that can be a serious pain to write and maintain. The solution is to apply what we learnt earlier about rethrowing caught exceptions:
try
{
return Int32.Parse(stringValue);
}
catch (Exception ex)
{
if (!(ex is FormatException) && !(ex is InvalidCastException) && !(ex is OverflowException))
{
throw;
}
return defaultValue;
}
While it's true I said that you should never catch Exception, notice that it's being rethrown (using
throw;) if it isn't the expected type. This works just as well in VB.NET, or you can even use VB.NET's when clause.
Global Error HandlingIf you've made it this far, you might be getting a little panicked. Since we aren't catching exceptions left and right, they'll bubble until they crash the system. Well that's where global exception handling comes in. It lets us write, in a centralized (i.e., easy to change) way, any exception logging we want and how to display the error in a friendly manner. Now I'm getting a little tired of typing, but here's the httpModule I use to get the job done (notice it relies on log4net to do any logging). If you aren't familiar with httpModules, and you don't want to bother learning, you can take the code from application_error and stick it in your global.asax's application_error.
using System;
using System.Web;
using log4net;
namespace Fuel.Web
{
public class ErrorModule : IHttpModule
{
#region Fields and Properties
private static readonly ILog logger = LogManager.GetLogger(typeof(ErrorModule));
#endregion
#region IHttpModule Members
public void Init(HttpApplication application)
{
application.Error += new EventHandler(application_Error);
}
public void Dispose() { }
#endregion
public void application_Error(object sender, EventArgs e)
{
HttpContext ctx = HttpContext.Current;
//get the inner most exception
Exception exception;
for (exception = ctx.Server.GetLastError(); exception.InnerException != null; exception = exception.InnerException) { }
if (exception is HttpException && ((HttpException)exception).ErrorCode == 404)
{
logger.Warn("A 404 occurred", exception);
}
else
{
logger.Error("ErrorModule caught an unhandled exception", exception);
}
ctx.Server.ClearError();
//if you want, you can simply redirect to a generic "oops, an error occurred page" ala:
//this is what I recommend until you get into creating your own exceptions
//Response.Redirect("error.asx");
}
}
}What if you disagree with me?You might disagree with what I've said. You might even point to this MSDN reference that supposedly tells you how to handle exception:
http://msdn2.microsoft.com/en-us/library/24395wz3.aspx. In it you'll find very different advice, namely:
"It is preferable to use Try/Catch blocks around any code that is subject to errors rather than rely on a global error handler."
I'll counter that crap (and i am trying to get it corrected, because it IS flat out wrong), with a quote from
Anders Hejlberg (you know, the lead architect for C#, one of few Microsoft distinguished engineers, the inventor of TurboPascal and a lot more):
"No, because in a lot of cases, people don't care. They're not going to handle any of these exceptions. There's a bottom level exception handler around their message loop. That handler is just going to bring up a dialog that says what went wrong and continue. The programmers protect their code by writing try finally's everywhere, so they'll back out correctly if an exception occurs, but they're not actually interested in handling the exceptions.
The throws clause, at least the way it's implemented in Java, doesn't necessarily force you to handle the exceptions, but if you don't handle them, it forces you to acknowledge precisely which exceptions might pass through. It requires you to either catch declared exceptions or put them in your own throws clause. To work around this requirement, people do ridiculous things. For example, they decorate every method with, "throws Exception." That just completely defeats the feature, and you just made the programmer write more gobbledy gunk. That doesn't help anybody.
It is funny how people think that the important thing about exceptions is handling them. That is not the important thing about exceptions. In a well-written application there's a ratio of ten to one, in my opinion, of try finally to try catch. Or in C#, using statements, which are like try finally.
In the finally, you protect yourself against the exceptions, but you don't actually handle them. Error handling you put somewhere else. Surely in any kind of event-driven application like any kind of modern UI, you typically put an exception handler around your main message pump, and you just handle exceptions as they fall out that way. But you make sure you protect yourself all the way out by deallocating any resources you've grabbed, and so forth. You clean up after yourself, so you're always in a consistent state. You don't want a program where in 100 different places you handle exceptions and pop up error dialogs. What if you want to change the way you put up that dialog box? That's just terrible. The exception handling should be centralized, and you should just protect yourself as the exceptions propagate out to the handle."
(you can read the entire interview
here, very good read, especially if you come from a Java backgorund)
Make sure to read that and understand that well, because it's all amazing advice and the real foundation of what you need to know with respect to exception handling.
In Closing:Here are the key points:
- Don't catch exceptions unless you can actually handle them
- Do know how to rethrow exceptions properly
- Do use using or try/finally often
- Don't swallow exceptions
- Do use a global handler to help you log and display a friendly error message
I did want to talk about creating your own exceptions, but I think I'll save that for another post.