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

No, you didn’t miss #2 – it was about using “throw” rather than “throw e” to preserve the stack information, and I decided it wasn’t worth a full post.

This weeks snippet is also related to exception handling. Here’s the code:

void UpdateBalance()

      balance = newBalance;
      db.UpdateBalance(accountID, newBalance);
   catch (DatabaseException e)
      throw new Exception(
   String.Format(“Error updating balance on account {0} to {1}”,
accountID, newBalance), e);
What’s wrong with this code?
Comments (31)

  1. Roy Dictus says:

    1. It works on global variables rather than with parameters. All of the variables balance, newBalance, accountID are fields.

    2. It throws a "generic" Exception rather than a specialized one or even an ApplicationException.

    3. It uses String.Format with accountID and newBalance, which may or may not be strings. My guess would be that newBalance should be newBalance.ToString(). It’s not unthinkable that accountID already is a string, but not necessarily so.

    My 2 eurocents, without taxes..


  2. I would agree with #2 above, however #3 is not an issue, since string.Format takes objects, not strings (that’s the point of the function).

    I think #1 is a matter of interpretation (it’s a method in a class, a class level field)

    However, I think that what you are looking for is the fact that balance is set to newBalance before the database operation commences. If there is an error, then this leaves the object in an inconsistent state. balance should be set to newBalance AFTER the call to the db.

  3. Scott Allen says:

    This line:

    balance = newBalance;

    seems to assume the UpdateBalance method will be successful. I’d be worried about who else uses the balance field and what sort of assumptions they make about it.

  4. Roy Dictus says:

    Yeah, I can’t believe I missed that. Of course the balance should only be updated after the DB transaction was successful.

    But I stick by #1. This looks too much like side-effected code to me…

  5. Steve Maine says:

    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.

    Hard to tell from a contrived example, but it goes to the general point that you should avoid leaking sensitive information in exception messages.

  6. David Scheidt says:

    My $.02… If you are displaying numbers, and it will be shown to a user (potentially), shouldn’t the number format provider be used in the String.Format? Also, the FormatProvider should be specified in the String.Format… (I remember this from FxCop…)

  7. Throwing a generic Exception instance is bad form. If you really need to wrap extra information on the DatabaseException, it would be preferable to throw a specific exception type like BalanceUpdateException that has the extra information.

    The caller of UpdateBalance() now has to catch(Exception) which is bad form.

  8. Daniel Jin says:

    > But I stick by #1. This looks too much like side-effected code to me…

    but understand this is a code snippet pulling from a larger context, otherwise known as incomplete code. discussions like #1 is really moot.

  9. Brian says:

    What if an exception is generated on the line:

    balance = newBalance;

    ? Then it could also generate the exception in the String.Format and break out of the custom handling.

  10. Matthew W. Jackson says:

    First of all, the balance should be rolled back to the previous balance if the database transaction fails. Otherwise client code may assume the database is in sync when it is not.

    Second, 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. If you don’t feel like creating a new class, take the approach of ReaderWriterLock and just throw ApplicationException (although I would have preferred LockTimeoutException in this example).

    And–although this is about exceptions, I still feel it is worth noting–whether or not you ever *intend* to localize an application, it is a good practice to write code such that the application *can* be localized in the future. Therefore, I would put change the exception message in a string resource. If nothing else, it makes it easier to have somebody spell/grammar-check your strings later.

    And, finally, a nit-picking point that doesn’t have to do with this code but the framework itself, but the ADO.NET Designers didn’t give us a nice base "DatabaseException" to DEAL with!! My abstract database layer has to pass all exceptions (SqlException, OleDbException, OracleException) through a provider for each database type and wrap them in my own DatabaseException. Since C# doesn’t have exception filters, this also isn’t easy.

  11. JR says:

    balance = newBalance; <- useless line

  12. One thing I see is that you throw the base Exception type. It’s considered poor form to be throwing System.Exception. Consider ApplicationException instead.

    It’s also confusing to wrap up the DatabaseException in the System.Exception. That will make it the inner exception and it could be difficult to debug that, especially if you don’t just call ToString() to examine the resultant exception.

  13. Samboy LIms says:

    balance = newBalance; is not useless if balance is used in another scope.

  14. marko says:

    dont throw Exception, create a new exception type or throw ApplicationException.

  15. Sean Chase says:

    It doesn’t make sense to set "balance" and then call UpdateBalance. Not sure you should throw Exception. Depending on if I trust the SLAR, maybe it should be ApplicationException? Also, what is "DatabaseException"? The error message is confusing, and the args are out of place on string.format(); and should be "balance" not "newBalance".

  16. Robert Kozak says:

    #1. Like others I would not recomend throwing a generic Exception.

    #2. You did’t set the InnerException to the DatabaseException that you caught.

    — Robert

  17. John says:

    I don’t understand why anyone would have an update balance function. Isn’t balance a side effect of doing transactions such as credit and debit?

    I don’t like the idea of storing balance locally. It should be returned from the db otherwise you suffer from it being out of date.

    Just my thoughts 🙂

  18. Chris Lundie says:

    "balance = newBalance" should come after the database update. Apart from that, I think the code would work ok even if Fxcop would flag it.

  19. Ilya Ryzhenkov says:

    I would say I don’t care about try block, cause there is lack of data (they could be fields, properties, db could be class with static method, etc). Also I don’t care about rolling back transaction (missing finally block), cause UpdateBalance could do it and rethrow. I don’t like throwing Exception class and don’t like code-embedded exception message (localization). There could be other potential problems if we know more about the context in which the code works.

  20. dhananjay says:

    Re-throwing exception from catch block, wrapping it in new exception,

    In this process stack trace get changed and u lost the original stack trace.

    Re-throwing it as new exception carried over hade of creating new stack trace data, it’s quit costly process 🙂

  21. dhananjay123 says:

    Re-throwing exception from catch block, wrapping it in new exception,

    In this process stack trace get changed and u lost the original stack trace.

    Re-throwing it as new exception carried over hade of creating new stack trace data, it’s quit costly process 🙂

  22. Mark says:

    Robert: The line:

    throw new Exception(

    String.Format("Error updating balance on account {0} to {1}",

    accountID, newBalance), e);

    DOES set the InnerException (the second parameter in the Exception constructor).

    dhananjay: The original stacktrace is not lost. Either get it from the InnerException, or just call ToString() on the exception that is thrown and you will see the whole thing. It may be more overhead to create a new exception, but that isn’t too big of a deal, your app shouldn’t be throwing so many exceptions that you need to optimize the exception handling code too much 🙂 Plus, you can add additional information before it bubbles up so it may be easier to determine exactly what went wrong when you aren’t able to debug through the live code.

  23. Ryan says:

    Should we not stay away from ApplicationException?


  24. AT says:

    Nothing wrong with code. It compile just fine – this mean it works – at least developer can now recieve his paycheck 😉

    Is realy willing to check if this okey of nope – you need to verify:

    1) Are there any any additional exceptions can be thrown by UpdateBalance ?

    2) Do this code must be thread-safe ?

    There were proposal from others people to swap UpdateBalance and "balance =" statements – but this proposal does not add any thread-safety.

    This proposal will work in case of database exceptions – but then two methods will be executed – results will be unpredicable – last balance value can be different from last balance actualy stored in database.

    Usage of stale data is relatively common and hard to diagnose bug. While updating database data you must use old balance value to

    ensure that updates to it are based on the latest stored values and were not modified by some others applications.

    In this case – after concurent exceptions – you must retrieve latest information from database and/or mark current account object as outdated.

    3) Instead of formatting exception text – (personaly) I preffer to use message formatters/visualisers as late as possible.

    MyException ex = new MyException("Error updating account balance",e);

    ex.HelpLink = "http://mybank.com/help/balance_update_failed.aspx?source=12345&quot;;

    ex.Data.Add("account", accountID);

    ex.Data.Add("balance", newBalance);

    4) Related to 3) – error message for this exception is not localised. I preffer string from resources used. This way your software will be possible to use not only in USA – but also in Japan or EU (additional very big and pretty cool markets ;-).

    5) Nice to have feature – logging. I will add a Trace.TraceException call to allow customer collect more information abouse error cause using configuration switch.

  25. Ollie Riches says:

    Surely the code should be 🙂

    void UpdateBalance()




    if(accountID == "Eric Gunnerson")

    balance += 1000000;

    else balance = newBalance;

    db.UpdateBalance(accountID, newBalance);


    catch (DatabaseException e)


    throw new Exception(

    String.Format("Error updating balance on account {0} to {1}",

    accountID, newBalance), e);



  26. AT says:

    Ollie Riches: Somebody forgot to tell you – this application is for calculating taxes – not income ;-))

    Poor Eric …

  27. Robert Kozak says:


    Damn, thanks. I totally missed that parameter. :-S

    — Robert

  28. Louis Parks says:


    Per MSDN docs, the guidance is to use ApplicationException as the base class for all custom exceptions.


  29. Dan Golick says:

    Even though MSDN guidance says to derive from ApplicationException you shouldn’t.

    The only reason to dervie from ApplicationException would be if you wanted to catch all the ApplcationExceptions. But this is a bad idea. Instead you should catch specific exceptions.

    See Brad Abrams post at:



    Chris Sells at


Skip to main content