In space, no one can hear you scream

Lately, most of my time is being spent refactoring legacy code. Everywhere I look I find try/catch blocks wrapped around a few lines of code. It appears the developer was using this as a way to "fix" bugs - by catching and eating the exception instead of tracking down the root cause. Enabling exception breakpoints and attempting to run the code is enough to make a developer scream. Unfortunately, if the exception isn't rethrown...

   catch (Exception)

   {

       // No one can hear you scream!

   }

I may have to put that on a t-shirt...

Proper exception usage

Continuing our FxCop code cleanup, I decided to focus the next set of rules on working with exceptions.

Do not raise reserved exception types

These exceptions (such as SystemException and OutOfMemoryException) were designed as base classes or for CLR use only. Instead of using one of these, either find a more specific one in the .Net Framework or create your own.

  public static void ThrowsBaseException()

  {

      // This is too vague to be useful

      throw new Exception("Bummer");

  }

Instantiate argument exceptions correctly

The following is an example violation. Note that the thrown exception doesn't inform the caller which argument was at fault or how it was supposed to be called.

  public static int Divide(int dividend, int divisor)

  {

      if (divisor == 0)

          throw new ArgumentException();

      return dividend / divisor;

  }

To fix this, use a constructor that takes the name of the parameter and/or a string message stating the problem.

  throw new ArgumentException("Divisor cannot be 0", "divisor");

Do not raise exceptions in unexpected locations

Certain methods are generally assumed to never throw an exception when called (equality operators) or only throw certain exceptions (such as property getters.) To be consistent, your code should follow a similar pattern.

For example, the debugger uses the ToString method to display information. The following will cause issues while debugging:

  public override string ToString()

  {

      throw new Exception("Don't do this!");

  }

Do not raise exceptions in exception clauses

The following code attempts to call the Divide method with a divisor of zero:

  public void ThrowsExceptionFromFinally()

  {

      try

      {

          Divide(12, 0);

      }

      finally

      {

          throw new Exception("Ouch");

      }

  }

Based on the method defined previously, the code should throw an ArgumentException. In this example, however, the ArgumentException is lost due to a new exception being thrown from the finally block. If you want to thrown a new exception, do so from a catch block and include the caught exception as the inner exception.

  catch (ArgumentException ex)

  {

      throw new Exception("Ouch", ex);

  }

Exceptions should be public

The previous rules dealt with exceptions built into the .Net Framework. The last two deal with custom exceptions. Take the following custom exception class:

  internal class MyCustomException : Exception

  {    

      public MyCustomException()

      {            

      }

      public MyCustomException(string message)

          : base(message)

      {            

      }

  }

Note that the class is marked internal. The problem with this is that outside the assembly, the only way to handle this exception is to catch the base Exception (which is a bad thing.) The simple fix is to make the class public.

Implement standard exception constructors

Using the above exception class, note there are currently two constructors. The base Exception, however, defines two others - one to allow inner exceptions and one for serialization. Correcting the violation means making sure all four standard constructors have been defined. To fix the above class, the following methods need to be added:

  public MyCustomException(string message, Exception innerException)

      : base(message, innerException)

  {            

  }

  protected MyCustomException(SerializationInfo info, StreamingContext context)

      : base(info, context)

  {            

  }