0 Comments

Exceptions is very fundamental concept in CLR and every developer uses them extensively, but I am still surprised how many times I see incorrect use of exceptions during code reviews. For example:

When is it appropriate to have an empty catch block? Too many times I see some variation of this:

try
{
    PerformOperation();
}
catch
{                    
    // Do nothing
}

In my opinion – this is always bad. At the very least there has to be some logging framework in place like log4net, or Enterprise Library or custom logging solution, or tracing. Besides, you cannot possibly anticipate every type of exception that PerformOperation() function might throw, so empty catch block is the equivalent of sweeping issues under the rug, which is careless and even dangerous.

How about this code?

try
{
    PerformOperation();
}
catch (Exception ex)
{                    
    throw ex;
}

Also very sloppy and absolutely not recommended. Rethrowing exception this way resets the starting point of the exception, so you loose all the valuable stack trace information that caused that exception in the first place. The appropriate way is to execute throw; And even then if exception becomes unhandled Windows will reset the stack’s starting point, so tough luck Sad smile. If this bothers you you can do the following suggestion straight from Richter’s book:

 

var trySucceeds = false;
try
{
    // method code goes here
    trySucceeds = true;
}
finally
{
    if (!trySucceeds)
    {
       // catch code goes here
    }
}

So, going back to the previous issue, here is the question: is the code below correct?

try
{
    PerformOperation();
}
catch (Exception)
{                    
    throw;
}

 

And the answer is – not really.

Even though the stack trace is preserved (possibly) and exception is re-thrown. There is no logging statement, there is no code to reset data into its original state, therefore there is no value in having try / catch block around that function. Let that exception bubble up the stack. Thus try / catch block in this instance is totally unnecessary.

This is important because it is unusual to have a single layer under your client code or UI. Usually there would be some dependency on a service layer or a database layer, so you call a function with try / catch that calls a function with try / catch that calls another one etc. And if every one of these layers catches everything … – you get the picture.

For example. Let’s say you have your logging framework in place. And let’s say the requirement is not to show any exceptions to the user, or there is no user, because you have some process calling your code so you do need to catch everything. So you write something like:

// Some logic on UI thread calling WCF service 
try
{
      var result = _serviceModel.CallService();
      // ...
}
catch (Exception ex)
{
     ProcessException(ex);
}

// then somewhere in ServiceModel class ... 
public string CallService ()
{
     try
     {
          OpenChannel();
          return Client.GetData();
     }
     catch(Exception ex)
     {
          LogException(ex);
          throw;
     }
     finally
     {
          CloseChannel();
     }
}

So “catch all” statements are bad but nested “catch all” statements are even worst.

 

My point is: that in case when you absolutely have to have “catch all” statement, then keep it on the very top level, everything else either catch a specific exception or let it propagate up the stack. So this should look like:

// Some logic on UI thread calling WCF service 
try
{
    var result = _serviceModel.CallService();
}

// hopefully the service is wrapping exceptions into specific faults. 
catch (FaultException ex)
{
     ProcessFault(ex);
}
catch (Exception ex)
{
     ProcessException(ex);
}

The lower code should be either this:

public string CallService ()
{
     try
     {
          OpenChannel();
          return Client.GetData();
     }
     finally
     {
          CloseChannel();
     }
}

Or this:

public string CallService ()
{
     try
     {
          OpenChannel();
          return Client.GetData();
     }
     catch(CommunicationException ex)
     {
          LogException(ex);
          return null;
     }
     finally
     {
          CloseChannel();
     }
}

There is a difference. When you catching specific exceptions you anticipate the failure, so there is no reason to throw it again. So catching everything statement should be reserved for unexpected faults only.

Same thing when making database calls, i.e. only catch specific exceptions like DbException or SQLxception and let the others bubble up.

Bottom line: catching everything is bad, catching everything inside the dependent layers of your code is worst, and having empty catch statement is just evil.

Cheers.