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()
{

   try

   {

      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

• Anonymous
  November 18, 2004

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..
   
   Roy

• Anonymous
  November 18, 2004
  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.
• Anonymous
  November 18, 2004
  The comment has been removed
• Anonymous
  November 18, 2004
  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...
• Anonymous
  November 18, 2004
  The comment has been removed
• Anonymous
  November 18, 2004
  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...)
• Anonymous
  November 18, 2004
  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.
  
  
• Anonymous
  November 18, 2004
  > 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.
• Anonymous
  November 18, 2004
  The comment has been removed
• Anonymous
  November 18, 2004
  The comment has been removed
• Anonymous
  November 18, 2004
  The comment has been removed
• Anonymous
  November 18, 2004
  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.
• Anonymous
  November 18, 2004
  balance = newBalance; is not useless if balance is used in another scope.
• Anonymous
  November 18, 2004
  dont throw Exception, create a new exception type or throw ApplicationException.
• Anonymous
  November 18, 2004
  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".
• Anonymous
  November 18, 2004
  #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
• Anonymous
  November 18, 2004
  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 :-)
• Anonymous
  November 18, 2004
  "balance = newBalance" should come after the database update. Apart from that, I think the code would work ok even if Fxcop would flag it.
• Anonymous
  November 18, 2004
  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.
  
• Anonymous
  November 18, 2004
  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 :)
• Anonymous
  November 18, 2004
  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 :)
• Anonymous
  November 18, 2004
  The comment has been removed
• Anonymous
  November 18, 2004
  Should we not stay away from ApplicationException?
  
  http://blogs.msdn.com/brada/archive/2004/03/25/96251.aspx
• Anonymous
  November 18, 2004
  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";
  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.
• Anonymous
  November 18, 2004
  Surely the code should be :)
  
  void UpdateBalance()
  {
  try
  {
  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);
  }
  }
• Anonymous
  November 18, 2004
  Ollie Riches: Somebody forgot to tell you - this application is for calculating taxes - not income ;-))
  
  Poor Eric ...
• Anonymous
  November 19, 2004
  The comment has been removed
• Anonymous
  November 21, 2004
  Ryan,
  
  Per MSDN docs, the guidance is to use ApplicationException as the base class for all custom exceptions.
  
  http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpref/html/frlrfsystemapplicationexceptionclasstopic.asp
  
• Anonymous
  November 23, 2004
  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:
  http://blogs.msdn.com/brada/archive/2004/03/25/96251.aspx
  
  and
  
  Chris Sells at
  
  http://www.sellsbrothers.com/news/showTopic.aspx?ixTopic=1274
• Anonymous
  December 27, 2004
  [http://itpeixun.51.net/][http://aissl.51.net/][http://kukuxz003.freewebpage.org/][http://kukuxz001.51.net/][http://kukuxz003.51.net/][http://kukuxz005.51.net/][http://kukuxz002.51.net/][http://kukuxz004.freewebpage.org/][http://kukuxz007.51.net/][http://kukuxz001.freewebpage.org/][http://kukuxz006.51.net/][http://kukuxz002.freewebpage.org/][http://kukuxz004.51.net/][http://kukuxz008.51.net/][http://kukuxz009.51.net/][http://kukuxz005.freewebpage.org/][http://kukuxz006.freewebpage.org/][http://kukuxz007.freewebpage.org/][http://kukuxz009.freewebpage.org/]