Catching Exception is almost never justified and almost always harmful

Posted at 08:00 on 04 February 2010

I was doing an ad-hoc review of another developer's code not long ago when I saw something like this:

try {
    return bool.Parse(GetSomething());
}
catch (Exception) {
    return false;
}

I gently pointed out to him that this is a bad practice. Apart from the fact that you can use bool.TryParse() instead of bool.Parse(), your GetSomething() method may be throwing exceptions indicating a rather more serious problem, such as your database being down.

Catching Exception is one of my pet peeves, but sadly it's far too common, even among smart developers that I'd have expected to know better, cropping up in commercial products and open source projects alike. Part of the problem is the code samples in the MSDN documentation itself, which are littered with completely unnecessary try ... catch (Exception) blocks, that people copy and paste without thinking about it. But it's also a quick and dirty hack -- it's easier to simply catch Exception and cross your fingers than to look up the documentation to find out exactly what you should be catching.

But this is reckless and dangerous. Catching exceptions inappropriately can lead to some very serious bugs in your code -- serious, because you are deliberately ignoring them while they wreak havoc with your data. In one instance, I was asked to troubleshoot an application where a database upgrade had been botched and nobody had noticed for several days until the users started complaining that their changes weren't being saved. You may also be ignoring misconfiguration, missing assemblies, external services being offline, and so on. And even if the effects aren't serious, the bugs can still be particularly difficult to track down, as your logs will likely contain misleading error reports, if indeed they contain any error reports at all.

Catching general exception types without re-throwing them is almost never justified, and almost always harmful.

The correct approach to exceptions is to allow them to bubble up to the topmost level of your code, and handle them there by logging them and presenting an approriate error message to the user. For ASP.NET applications, this is the Application_Error event handler in your global.asax file, or perhaps an error logging framework such as ELMAH. For console applications, it is your Main method. For separate threads, it is the topmost method of the thread. And so on.

Well written code has very few try ... catch blocks. The most common case where you would have a general exception handler is when you need to roll back a transaction or otherwise leave your application in a consistent state when you re-throw:

try {
    BeginTransaction();
    DoStuff();
    Commit();
}
catch {
    RollBack();
    throw;
}

Aside: when you re-throw the exception, always use throw; here (which preserves the stack trace), not throw ex; (which doesn't).

Apart from that, you should only catch specific exception types that you are both able and willing to handle meaningfully. Certainly, catching Exception should be treated as the nuclear option -- and if there really is no alternative, you should always log the exceptions and rigorously justify your decision both in comments and in a code review. And next time you are tempted to write catch (Exception), ask yourself this question:

What would this code do if the exception were due to a botched deployment, an out of memory error, or a misconfiguration?