Correctly disposing of objects - implement IDisposable

To continue with our FxCop backlog, we are going to look at a couple rules dealing with IDisposable. Consider the following class:

  public class TimedProcessor

  {

      Timer startTimer = new Timer();

      public TimedProcessor(double interval)

      {

          startTimer = new Timer(interval);

          startTimer.Elapsed += timer_Elapsed;

      }

      void timer_Elapsed(object sender, ElapsedEventArgs e)

      {

          Console.WriteLine("Do work here");

      }

  }

Running FxCop on this will report a violation of the rule Types that own disposable fields should be disposable. The problem is that the startTimer field is of type Timer, which implements IDisposable. To properly use the timer, we need to call its Dispose method as soon as we are done with it. The solution is to implement IDisposable on our class and make sure we call startTimer.Dispose().

The class after fixing the violation:

  public class TimedProcessor : IDisposable

  {

      Timer startTimer = new Timer();

      public TimedProcessor(double interval)

      {

          startTimer = new Timer(interval);

          startTimer.Elapsed += timer_Elapsed;

      }

      void timer_Elapsed(object sender, ElapsedEventArgs e)

      {

          Console.WriteLine("Do work here");

      }

      public void Dispose()

      {

          if (startTimer != null)

              startTimer.Dispose();

      }

  }

Note that I check for null before calling Dispose. If an error elsewhere left the object in an invalid state, we don't want our Dispose method to throw a NullReferenceException.

Now, say we add a second field called endTimer. Running FxCop now will report a violation of the rule Disposable fields should be disposed. In this case we have already implemented IDisposable, but not all of our disposable fields have been addressed. To fix this, we need to modify our Dispose method slightly:

  public void Dispose()

  {

      if (startTimer != null)

          startTimer.Dispose();

      if (endTimer != null)

          endTimer.Dispose();

  }

A custom FxCop rule - calling Debug methods

While working on a recent production issue, we ran into an interesting problem. We suspected an exception was being thrown when calling an external site, but we couldn't prove it. Our usual method of exception handling is to save the details to a rolling log file. This particular service, however, was failing without leaving behind any info as to why. Digging through the code revealed a catch block similar to this:

  catch (Exception ex)

  {

      Debug.WriteLine(ex.ToString());

  }

For now, ignore the fact that you will only see output using something like DebugView. The real problem is that when the code is compiled in Release mode, calls to Debug.WriteLine are removed completely. So in the example above, the catch block will be empty.

To reduce the odds of this sort of thing happening in the future, I decided to write a custom FxCop rule to locate any calls to methods that had been tagged with a "Debug" conditional. It wasn't as easy as I had expected, but there is an excellent tutorial on the subject. The only difficulty I had was in locating the ConditionalSymbol property in the class tree.

For anyone interested, here is the xml rule file:

    <?xml version="1.0" encoding="utf-8" ?>

    <Rules FriendlyName="Custom Rules">

        <Rule TypeName="DoNotCallDebugConditionalMethods" Category="CustomRules" CheckId="PG1001">

            <Name>Do not call debug conditional methods</Name>

            <Description>Calls to methods marked with the DEBUG conditional will be removed

            from Release builds.</Description>

            <Url></Url>

            <Resolution>Replace the call to '{0}' with a more appropriate call</Resolution>

            <MessageLevel Certainty="100">Warning</MessageLevel>

            <Email></Email>

            <FixCategories>DependsOnFix</FixCategories>

            <Owner></Owner>

        </Rule>

    </Rules>

And the code:

  namespace CustomFxCopRules

  {

      /// <summary>

      /// Warns of any methods being called that are removed from non-debug builds,

      /// such as Debug.WriteLine()

      /// </summary>

      public class DoNotCallDebugConditionalMethods : BaseIntrospectionRule

      {

          public DoNotCallDebugConditionalMethods()

              : base("DoNotCallDebugConditionalMethods", "CustomFxCopRules.rules.xml",

                  typeof(DoNotCallDebugConditionalMethods).Assembly)

          { }

          public override ProblemCollection Check(Member member)

          {

              Method method = member as Method;

              if (method != null)

              {

                  VisitStatements(method.Body.Statements);

              }

              return Problems;

          }

          public override void VisitMethodCall(MethodCall call)

          {

              var member = ((MemberBinding)call.Callee).BoundMember;

              var method = (Method)member;

              var symbol = method.ConditionalSymbol;

              if (!string.IsNullOrEmpty(symbol) && symbol.Contains("DEBUG"))

              {

                  Problems.Add(new Problem(GetResolution(method.FullName), call.SourceContext));

              }

          }

      }

  }

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)

  {            

  }

Easy pickings: Class-level FxCop warnings

The last batch of FxCop warnings proved to require a lot more effort than I had anticipated. It's amazing how much dead code accumulates over the years. For the next set of rules I decided to pick ones that were easy to fix but, more importantly, had few actual violations in our projects.


Abstract types should not have constructors

The following class violates the rule:

  public abstract class MyAbstractClass

  {

      public MyAbstractClass()

      {

          // Do setup code for derived classes

      }

  }

A class defined as abstract can't be instantiated directly. Thus, the only purpose for a constructor is to allow for default setup when a derived class is created. To fix the issue, change the constructor accessibility to protected.


Do not declare protected members in sealed types
Do not declare virtual members in sealed types

A sealed class is one that cannot be used as a base class. Protected and virtual members are useful for derived classes, which is a contradiction. Fix the protected member by making it private instead. As for the virtual members, this is a C++ issue only, as C# and VB.Net will fail to compile. To fix this for C++, unseal the class or remove the virtual modifier.

  public sealed class MySealedClass

  {

      protected void Process()

      {

          // Might as well be private

      }

      public virtual void WillNotCompile()

      {

          // A C++ feature only, so that was easy :)

      }

  }

Static holder types should be sealed
Static holder types should not have constructors

Take the following:

  public class MyStaticClass

  {

      public static void DoWork()

      {

          // Do stuff here...

      }

  }

This class only contains static members, so there is no reason to create instances of the class. When this is compiled, however, the class will be given a default public constructor. Prior to .Net 2.0, the fix was to implement an empty constructor and set the access level to private. Beginning with .Net 2.0, an easier fix is to set the class itself to static.

Unit test all enum values with NUnit

In an older post I demonstrated NUnit's built-in parameterized tests feature. This allows a developer to call a single method to run multiple tests.

Let's say I want to run the test for each value in an enumeration. Using the TestCase attribute, I can write the test like this:

  [TestCase(Powertools.Chainsaw)]

  [TestCase(Powertools.CircularSaw)]

  [TestCase(Powertools.PowerDrill)]

  public void PowerToolsTestExplicit(Powertools p)

  {

      // Do test

  }

Which is fine, but what if I add a new value to the enum? Instead of having to add another attribute to the test, it would be easier to loop over all enum values at runtime. With the TestCaseSource attribute I can do just that.

Within my unit test class I first create a method that returns an IEnumerable (in this case Array) containing the enum values:

  public Array GetPowerTools()

  {

      return Enum.GetValues(typeof(Powertools));

  }

Then I create my unit test and decorate it with the TestCaseSource attribute. The attribute constructor takes one parameter, sourceName, which is the name of the method to call:

  [TestCaseSource("GetPowerTools")]

  public void PowerToolsTestWithIEnumerable(Powertools p)

  {

      // Do test

  }

In either case, this expands my unit tests as expected. The second method is easier to maintain and less likely to allow untested code into the system.

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

FxCop: A starting point

In an effort to improve overall code quality at work, we are initiating manual and automated code reviews. One of the tools we will be using for automated reviews is FxCop. There are just two minor issues with this:

1. Running every rule on an existing code base usually results in a massive backlog
2. Most developers, myself included, are unfamiliar with at least some of the rules

Granted, several of the rules categories can be turned off on most projects (Portability and Interoperability come to mind.) There are also a few rules that are unimportant or largely obsolete. This still leaves a sizeable list of rules to deal with.

Our planned approach is to enable several of the rules as warnings on the build server. After the developers have addressed any issues, the build will be modified to fail on future violations of those rules. Slowly adding rules, first as warnings and then as errors, will allow us to clean up our existing code base and prevent new violations from being introduced. As we go I will be putting together examples of violations and fixes. I'll post the examples for those who want to follow along at home.