Remove unused code

When considering the next set of FxCop rules to enable on the build server, my first thought was to look at those dealing with properly disposing of objects. Scanning the active projects at work has revealed a rule-set slightly more important - unused code. In some of our older code bases there are a variety of methods, variables, etc. that are no longer being used. Deleting the dead code will make it easier to understand what the code is actually doing. In many cases, this will also fix a variety of other FxCop issues, as the flagged items are no longer present. Why fix what you will eventually delete?

And before I go over the rules I should emphasize deletion of dead code. I've seen numerous instances where a developer has commented out, #ifdef'd, or otherwise excluded a section of code. In some cases this was to finish or re-implement the code later. In other cases it was to remove code not currently needed. Regardless, this should be avoided. Tracking previous revisions of code is what source control is for.

The first rule to address is Avoid uninstantiated internal classes. This is a class not visible outside the assembly and is never actually used within that assembly. If you're lucky, deleting this class will improve a variety of code metrics (the number of FxCop warnings being just one of them.)

For the next examples I reference the following class:

    9   public class Unused

   10   {

   11       int unusedPrivateField;

   12 

   13       private void UnusedPrivateCode()

   14       {

   15           Debug.WriteLine("This method isn't called");

   16       }

   17 

   18       internal void DoWork()

   19       {

   20           int unusedLocal;

   21           Debug.WriteLine("This is the only method possibly called");

   22       }

   23   }

Working through the code, line 11 returns a warning for the rule Avoid unused private fields. Notice that none of the methods use it and it isn't exposed through a property.

Next, the method UnusedPrivateCode violates, you guessed it, Avoid uncalled private code. This method is uncalled locally and unreachable outside the class.

The final violation is of type Remove unused locals, at line 20. The variable is not used within the method and can be removed. Note that this rule will also fire if the variable is assigned to, but its value is never actually used. An example:

   18   public void DoWork()

   19   {

   20       int unusedLocal = 0;

   21       unusedLocal = 12; // Still not using...

   22   }

Rethrowing exceptions to preserve stack details

In my last post, I described our plan at work to introduce FxCop into our development process. The first rule we will be enforcing is RethrowToPreserveStackDetails.

The following example violates the rule:

   12   public void MyTest()

   13   {

   14       Process();

   15   }

   16 

   17   public void Process()

   18   {

   19       try

   20       {

   21           int result = Calculator.Divide(12, 0);

   22       }

   23       catch (DivideByZeroException ex)

   24       {

   25           // React, likely by logging

   26           throw ex; // <- This is wrong

   27       }

   28   }

Executing this code results in the following logged callstack

  ...FxCopTests.Process() in C:\test\FxCopTests.cs:line 26
  ...FxCopTests.MyTest() in C:\test\FxCopTests.cs:line 14

Note that the callstack ends with line 26, which is in the catch block. In this example the real exception location isn't hard to find. Unfortunately, production code is rarely this simple.

The problem here is that calling "throw ex" causes the callstack info to be created at that point. If you were creating a new exception and throwing it this would be desired behavior. With an existing exception you don't want the original callstack to be overwritten.

The fix for this code is easy enough - replace:

  throw ex;

with

  throw;

After this change, the callstack correctly points to the line throwing the exception

  ...Calculator.Divide(Int32 x, Int32 y) in C:\test\Calculator.cs:line 11
  ...FxCopTests.Process() in C:\test\FxCopTests.cs:line 26
  ...FxCopTests.MyTest() in C:\test\FxCopTests.cs:line 14