Ask dotNetDave – Properly Throwing & Rethrowing Exceptions

Recently at a contract I was 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 that I analyzed. 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.