What’s wrong with this code? (#3) – Answer

Here’s the discussion for the my last post.

I liked Matthew’s response:

…don’t throw “Exception” since you are not supposed to catch “Exception.” Anybody handling exceptions thrown by this code would have to catch Exception and ignore asynchronous exceptions (like OutOfMemoryException) at the same time, which is not easy.

To expand a bit, if you wrap in a class like Exception, you force the user to write something like:

catch (Exception e)
   if (e.InnerException.GetType() == typeof(DatabaseException))
      // handle the exception here

This is really ugly, and if the user forgets the final “throw”, the exception gets swallowed.

I also liked Steve’s comment:

I don’t like the fact that the exception message exposes both my account number and the account balance. If this exception text made it all the way back to the client, the current message seems like a privacy/security hole.

There are also likely things that need to be done to make the database part more robust, though that wasn’t what I was intending to illustrate.

Comments (7)

  1. Marcelo says:

    You might want to change

    if (e.InnerException.GetType() == typeof(DatabaseException))


    if (typeof(DatabaseException).IsAssignableFrom(e.InnerException.GetType()))

    This lets the called code throw a DatabaseException subclass in the future with more information, if they so wish, without breaking your code. Exact type comparison is rarely what people need in most situations…

  2. AT says:


    e.InnerException can be a null.

    Much better is


  3. AT says:

    Or the best one

    if (e.InnerException is DatabaseException) 😉

  4. Eric Newton says:

    Simplest is best…

    if( e.InnerException is DatabaseException )

    { … }

  5. David Levine says:

    Actually, looking at the InnerException is also error prone…there’s no guaranteee that there is a single InnerException as there is no limit to the number of catch-wrap-throw’s that occurred, so there is no way to determine in advance which InnerException you should examine. If you really want to look at the root cause you can use e.GetBaseException(), but again, there’s no guarantee that this will be what you want – the ultimate root cause may be a null reference which gets translated into a DatabaseException by a technical layer.

    This reinforces Eric’s original point – if a method throws recoverable exceptions then it should be of a type other then System.Exception. It should be a specific type so that a caller is never forced to examine inner exceptions, only the outermost exception. The inner exceptions should provide context and clarify the situation.

    As a side note, one could also argue that this ambiguity reflects a fundamental weakness in the current exception model and its implementation.

  6. MBA says:

    Helpful For MBA Fans.