james mckay dot net

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?

Comments closed (1) »

Posted at 08:00 on 4 February 2010.
Tagged: C#, exception handling

A few years ago, I inherited a VB.NET application in which every method (many of which were four hundred lines long, copied and pasted all over the place, and peppered with vague sounding variable names such as blnRunIf and blnRunElse) contained this boilerplate code:

Sub MyMethod()
    Try
        '
        ' ** snip ** '
        '
    Catch ex As Exception
        Throw New Exception ("MyClass.MyMethod::" + ex.Message)
    End Try
End Sub

Those who do not understand Exception.StackTrace are doomed to reinvent it, badly.

Comments closed »

Posted at 08:00 on 11 May 2009.
Tagged: exception handling, stupidity, VB.NET

MbUnit allows you to run assembly-level setup and teardown methods as part of your unit tests using the AssemblyCleanUpAttribute:

[assembly: AssemblyCleanUp(typeof(AssemblyCleaner))]
public class AssemblyCleaner
{
    [SetUp]
    public static void SetUp()
    {
        // blah
    }
    [TearDown]
    public static void TearDown()
    {
        // blah
    }
}

This is very useful if you want to do something like restore your database to a known configuration, perhaps incorporating all your change scripts into your unit tests. Unfortunately, there is a little gotcha. If your SetUp() method throws an exception, none of your unit tests will run, but MbUnit will still report success.

For what it’s worth, I think this is a bug, not a feature, but there is a way round it. Capture any exception, and create a unit test that re-throws it:

[assembly: AssemblyCleanUp(typeof(AssemblyCleaner))]

public class AssemblyCleaner
{
    private static Exception setupException = null;

    [SetUp]
    public static void SetUp()
    {
        try {
            // blah
        }
        catch (Exception ex) {
            setupException = ex;
        }
    }

    [TearDown]
    public static void TearDown()
    {
        // blah
    }

    internal static void RethrowSetupException()
    {
        if (setupException != null) {
            // Wrap the original exception to preserve its stack trace
            throw new InvalidOperationException(
                "An error occurred when setting up the tests",
                setupException);
        }
    }
}

// You need to have your unit tests in a separate class.
// MbUnit doesn't like you including test fixtures
// in your assembly cleanup class.

[TestFixture]
public class AssemblyCleanerTest
{
    [Test]
    public void ReportSetupException()
    {
        AssemblyCleaner.RethrowSetupException()
    }
}

Comments closed »

Posted at 08:00 on 4 May 2009.
Tagged: exception handling, MbUnit, unit testing

The standard instruction that tends to get bandied about on when to throw exceptions is probably the most useless piece of non-advice that I have ever come across.

“You should only throw exceptions in conditions that are exceptional.”

What on earth is that supposed to mean? It’s like saying “You should only eat food that is edible.” And that doesn’t tell you, for instance, that Agaricus bisporus is edible but Amanita phalloides isn’t.

However, there are actually two very simple guidelines that tell you, in terms that are much easier to understand, exactly when to throw an exception and when to catch one. They are:

1. Throw an exception when your method can’t do what its name says it does.

Scott Hanselman blogged about this a while ago and it is probably the best piece of advice I’ve read on the subject:

If your method is called “Save” and it can’t Save, then throw. If it’s called DoSomething and it can’t DoSomething, throw. The idea is that the method name is a verb and a contract. It’s promising to do its best and if it can’t do it, it’s very likely exceptional.

It’s clear, unambiguous, to the point and easy to understand. Each method should do one thing, its name should say what that one thing is, and if it can’t do that one thing, that is when you throw an exception.

2. Don’t catch an exception unless you intend to do something about it.

Now the first rule is about throwing exceptions; it doesn’t say anything about catching them. However, Patrick Cauldwell, whom he links to, has this to say:

Never catch Exception

• If it’s not a problem you know about, delegate up the call stack, because you don’t know what to do about it anyway

Far too often I see code that catches and silently discards all exceptions. This may give your program a superficial appearance of being more robust, but in actual fact, it is merely masking over a problem that may need to be addressed and dealt with.

Remember that an exception means that something has gone wrong. Unless you know exactly what to do about it to recover from the situation, you should let it propagate up to the top level of your program, where you should log it, and, if appropriate, fire off an e-mail to the developers and/or systems administrator to let them know about it.

There may be times when you need to catch Exception. Transactions may need to be rolled back, resources may need to be disposed, and so on. However, unless you can recover from the situation that caused the exception in the first place, you should re-throw it. And even if you do swallow exceptions for any reason (and you shouldn’t do so unless you have a good reason), you should never do so silently: the very least you should do is record a warning in the application’s event log.

Comments closed »

Posted at 23:39 on 23 April 2008.
Tagged: conventions, exception handling