Consider this stack trace from an exception thrown by Entity Framework 6.1.3:

Lucky for me, I know which tiny change I made to the code I am working with to induce the exception, and therefore I am pretty confident that I know what caused it. Had I not been so fortunate, this stack trace would have provided almost no help at all with diagnosing what is actually wrong.

Let’s try and decompile the topmost method, GetStoreTypeFromName, which is the one that apparently calls .Single(..) on a sequence without any matches. It looks like this:

This, Ladies And Gentlemen, is in my humble opinion almost unforgivable: It is simply not OK to call .Single(..) on stuff without re-throwing the exception along with information on what we were looking for.

A much improved version of the method would have looked like this:

this way not losing the original exception, still providing information from the callsite telling the caller what’s wrong.

An even better version of the method would provide me with a list of available “store types”, which often provides some great hints about what is wrong:

The next method in the stack trace is called ConfigureColumn, and sadly if you decompile the method, it will be just as cheap with regards to helping us know what was wrong, even though it has all kinds of valuable things to tell us like:

  • which type was it trying to map
  • which property could not be mapped
  • which column in the database was it trying to map to

etc. etc. etc.

Please don’t write code like that.

A lesson in bad exception practice

2 thoughts on “A lesson in bad exception practice

  • 23. August 2017 at 08:22
    Permalink

    I agree, but I think that the reason that code like that gets written is that it’s easier to write the ‘bad’ code than it is to write the proper code. C# (and Java for that matter) lets you write bad code.

    In e.g. Haskell, the function corresponding to Single is Data.List.find, which doesn’t return a single element, but rather a ‘Maybe element’. It’s possible that the function will return an element, but it also may return Nothing, and all callers are forced by the compiler to deal with both cases. It’s much harder to write bad code in Haskell, because the bad code is not going to compile.

    F# occupies a middle ground. It enables you to write worse code than Haskell, but in general you have to go out of your way to write as bad code as C# lets you do.

    BTW, the call to GetStoreTypes could return null, so that should be handled as well. This makes the exception handling even more complicated. Your current suggestion with a try/catch block could lead to a misleading error messages, because it’ll say that the problem was that it couldn’t find a particular element, while the actual exception was a NullReferenceException.

  • 23. August 2017 at 11:06
    Permalink

    I tend to use .SingleOrDefault in my code and then test for the null and throw an exception.

    I do like this approach but I think I’d make some helpful extension methods just to reduce the ceremony involved. Perhaps (NB: pseudocode!)

    public static T SingleWithEx(this IEnumerable enumerable, Predicate pred, string exceptionMessage) {
    try {
    return enumerable.Single(pred);
    } catch(err) {
    throw new ArgumentException(exceptionMessage, err);
    }
    }

    This could also elegantly handle the null case as the awesome Mr Seemann points out.

    If the message was expensive then I’d make it a callback.
    The extension could also take a Func to help with the string.Join() calculations you did.
    Sure it might be a little less efficient, but this should be the unhappy, infrequent code path anyway.

Comments are closed.