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:

try
{
   account.UpdateBalance();
}
catch (Exception e)
{
   if (e.InnerException.GetType() == typeof(DatabaseException))
   {
      // handle the exception here
   }
   else
   {
      throw;
   }
}

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.