Ask dotNetDave – Properly Throwing & Rethrowing Exceptions

Recently at a contract I am working on, I received an email from one of the developers why I marked the code below that needs to be fixed because there are multiple throwing exception violations. Let’s see if you can spot them? There are other issues with this code, just stick to the issues dealing with exceptions.

try
{
    Id = Customer.GetByName(cust, request.CustomerName).CustomerID;
}
catch (DataException ex)
{
    if (ex.Number == DataException.DataErrorEnum.NoDataFound)
    {
        throw new DataException("Customer with the specified name not 
                                 found.", 
                         (int)DataException.DataErrorEnum.NoDataFound);
    }
    else
    {
        throw ex;
    }
}
catch
{
    throw new ApplicationException("Unable to create new customer");
}

How many issues did you find? Let’s take a look from the first issue on down. In this line of code:

throw new DataException("Customer with the specified name not found.", 
                        (int)DataException.DataErrorEnum.NoDataFound);

The issue with the throwing of a new DataException is that the original exception is not defined as the inner exception. Without doing so, the original message and stack trace is lost, which is very important when logging the exception in another block of code. I question their custom exception design by requiring callers of this code to know the meaning of the number that comes from the enumeration. I would have included the enum as a property type or just created multiple Exception types, one representing each number.

The second issue is in this line of code:

throw ex;

This violates Microsoft Design Guideline: (CA2200) Rethrow to preserve stack details. Rethrowing the exception as shown above will lose the stack trace defined in ex. Again, the entire stack trace is important when logging elsewhere. This code should be changed to simply this:

throw;

The third issue is in this line of code:

throw new ApplicationException("Unable to create new customer");

This violates Microsoft Design Guideline: (CA2201) Do not raise reserved exception types. The reserved exception types are the System.Exception, System.ApplicationException and System.SystemException because they do not give enough information to the calling code and possibly the user. Instead, either throw a more derived type that already exists in the framework, or create your own type that derives from Exception. These three exceptions should only be used in a try/catch block in the application layer when it’s unknown what exceptions might be thrown by the code being called. In the case of calling code in the .NET framework, they are all documented. A custom exception should have been used here.

Also, the following exception types are reserved and should be thrown only by the common language runtime: System.ExecutionEngineException, System.IndexOutOfRangeException, System.NullReferenceException and System.OutOfMemoryException.

Unfortunately, I found over 900 violations of this type in only 293K lines of code. There is a lot of refactoring that needs to be done! Laziness is got a good excuse for bad code design.

This and more code design can be found in my book “David McCarter’s .NET Coding Standards”.

Got a question for dotNetDave? Go here to ask your code question.

Advertisements

7 thoughts on “Ask dotNetDave – Properly Throwing & Rethrowing Exceptions

  1. What about the catch block that throws ApplicationException? A “catch everything” block is hardly a good idea. Perhaps the GetByName method (in this example) has design issues.

  2. 1) The catch-all is probably the worst problem with that code.

    2) The if condition gives a prime opportunity to introduce C# 6 exception filters with the `when` keyword. That’s the best alternative to rethrowing.

    3) The catch-all appears at first glance to handle all exceptions, but it does not handle the exceptions thrown or rethrown in the first catch block. That may or may not be expected behavior. Switching to an exception filter instead of rethrowing will cause DataExceptions when ex.Number != DataException.DataErrorEnum.NoDataFound to start being handled by the catch-all.

  3. Anything except a DataException will also lose the inner exception details. If you *really* wanted Pokemon exception handling (which is a bad idea, as A Visitor pointed out), you’d need to make that a “catch (Exception ex)” block to supply the inner exception to the thrown exception.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s