Visiting Exception Handlers

Scott Hanselmen recently posted a blog post titled Good Exception Management Rule of Thumb, Back to Basics Edition: 

 


public void HandleException(String strMessage)
{
   //Log to trace file only if app settings are true
   if (Properties.Settings.Default.TraceLogging == true)
   {
      try
      {
         TraceSource objTrace = new TraceSource("YadaYadaTraceSource");
         objTrace.TraceEvent(TraceEventType.Information, 5, strMessage.ToUpper());
         objTrace.Flush();
         objTrace.Close();
      }
      catch
      {
      //do nothing if there was an error
      }
   }
   throw new Exception(Environment.NewLine + strMessage);
}

The block is supposed to be the final stop for any and all exceptions in an application; if no other block of code handles it, this one will.  Scott asked his blog readers to post their thoughts in the comments, and I thought I’d do one better, and post my thoughts here.

Parameter and Method

 

  • Method: Why isn’t it static?
  • Parameter Name.  Faux-Hungarian notation blows.  It’s a statically typed language, so we already know that we’re expecting a string. 
  • Message? We pass messages in OO world. It’s just what we do.  What sort of message is it? (Hint: It’s an exception)
  • The parameter that comes in should be of type Exception, not string.
Conditional Logic
  • Explicitly checking for true is superfluous in the if statement.  If TraceLogging is a boolean property, then the entire equals and everything to the right can be taken out.
  • Why does the comment tell me exactly what the conditional logic tells me?
Try/Catch
  • The Try catch block shouldn’t be there.  If there’s nothing in the catch, then it’s going to catch everything and swallow it if anything goes wrong. Whoops, probably not what we wanted. It should be a try/finally block, with the Close() and Flush() happening on the finally block (the flush should happen first, then the close). I also believe that Close calls flush, so there’s no need to say flush then close. Just close will do.
  • Again, with the hungarian notation.  They made great ghoulash, but not so great programming prefixes.
  • Magic number: 5. What does 5 mean? I went to Kings Dominion with a few friends of mine last summer.  One of my friends was deathly scared of roller coasters, so I took it as a personal challenge to get her on a roller coaster.  We happened upon the Flight of Fear, an indoor coaster that you can’t see until you’re actually on the ride.  As we were waiting in line, she noted the sign that showed a Double-Diamond with a 5 in front of it.  She looked at me and asked 5 was the worst.  I replied that it was a 5 out of 10.  My point is, context means everything.  An enum to replace the 5 would be preferable. Since this is System.Diagnostics.Trace we’re talking about, 5 is the event Identifier.  Again, an enum would make it much simpler for maintanence programmers to remember what’s what.
  • What’s with the ToUpper()? Do you want your users to have a heart attack, or think that you’re yelling at them? HOW DOES THIS COME ACROSS?  Kill the ToUpper(), you’ll save yourself money.
Throwing Exceptions
  • When you throw new exceptions, you lose the Stack Trace coming up to that point, definitely not what the poster wanted, I’m sure.  
So now that we have the pain points with the original code down, let’s update it:

public static void HandleException(Exception ex)
{
   if (Properties.Settings.Default.TraceLoggingEnabled)
   {
      try
      {
         TraceSource trace = new TraceSource("YadaYadaTraceSource");
         trace.TraceEvent(TraceEventType.Information, Trace.ApplicationException, ex.Message);
        
      }
      finally
      {
         trace.Flush();
         trace.Close();
      }
   }
   throw;
}

 What do you think, is the new way better? I look forward to your comments.

Leave a Reply