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.

More than one way to sort a List<>

One operation you occasionally need to perform is sorting a generic list of objects. Often developers code handle with an inline delegate. If I wanted to sort a collection of boardgames by rating, my code might look like this:

  games.Sort(

      delegate(BoardGame a, BoardGame b)

      {

          if (a.Rating < b.Rating)

              return 1;

          if (a.Rating > b.Rating)

              return -1;

          return 0;

      });

Although this works, it tends to look a bit cluttered. It also increases a method's complexity and doesn't adhere to the idea of separation of concerns. If we only need to sort Boardgames in this one location, a better solution is to move the comparison code into a separate method in the same class. The new method looks like this:

  public int CompareBoardgamesByRank(BoardGame a, BoardGame b)

  {

      if (a.Rating < b.Rating)

          return 1;

      if (a.Rating > b.Rating)

          return -1;

      return 0;

  }

Our call to Sort looks like this:

  games.Sort(CompareBoardgamesByRank);

This makes the code much cleaner for a single sort. If we want to sort Boardgames by rating from multiple locations, we need a better place to store the comparison method. If we implement IComparable on our Boardgame class, we can create a CompareTo method on the class like so:

  public int CompareTo(object obj)

  {

      BoardGame b = (BoardGame)obj;

      if (Rating < b.Rating)

          return 1;

      if (Rating > b.Rating)

          return -1;

      return 0;

  }

Since the class now has a default comparison, we no longer need to specify a delegate when calling Sort - the CompareTo method will automatically be executed with an empty call:

  games.Sort();

What if we need additional comparison methods for our Boardgame? An easy way to handle this is to create static methods on the class, which will be available anywhere that Boardgame can be accessed. Here are a couple methods I've added to my Boardgame class:

  public static int CompareByName(BoardGame a, BoardGame b)

  {

      return string.Compare(a.Name, b.Name);

  }

  public static int CompareByRankAssending(BoardGame a, BoardGame b)

  {

      if (a.Rating > b.Rating)

          return 1;

      if (a.Rating < b.Rating)

          return -1;

      return 0;

  }

Which I can now pass into the Sort method:

  games.Sort(BoardGame.CompareByRankAssending);

Refactoring assemblies without breaking existing apps

Over time, most development shops identify common code that needs to be used across multiple projects. This code is eventually collected into a single assembly, usually something like Common.dll or Utilities.dll. In the beginning this is a decent way to eliminate code duplication. Over time, however, this single assembly becomes difficult to maintain.

At this point the obvious fix is to split the assembly into multiple smaller ones. Unfortunately, by then the single assembly is used on numerous projects. A major refactoring now requires extensive changes across all of these referencing applications. This realization usually stops any refactoring effort, leaving the utilities assembly to continue growing larger and more unwieldy.

To look at potential solutions, I've created a Utilities assembly with the following Logger class:

  namespace Utilities

  {

      public class Logger

      {

          public void LogError(string message)

          {

              Debug.WriteLine("Error: " + message);

          }

      }

  }

The goal is to move this class to a separate assembly called LoggingUtilities.

One solution is to use the TypeForwardedTo attribute. This is an assembly-level attribute that flags a specified class as having moved. To use this, I start by moving the Logger class to my new assembly. Note that I keep the same namespace as before - this is required for the forwarding to work.

Next I add a reference to LoggingUtilities within Utilities.

Finally, I open up AssemblyInfo.cs file in the Utilities project and add the following line:

  [assembly: TypeForwardedTo(typeof(Utilities.Logger))]

If I recompile the dlls and drop them in a folder with my existing application, it will continue to function even though the class has been moved.

This takes care of keeping the current compiled code running, but what about future versions? If I open up the source for one of my applications and attempt to compile, I now receive errors stating "The type or namespace name 'Logger' could not be found." It seems the redirection works at runtime but not at compile time. For someone not familiar with the previous refactoring, this could prove an interesting issue to track down.

In my opinion, there is a far better solution than using the TypeForwardedTo attribute. Going back to the original code, this time I copy the code to the new assembly (as opposed to moving it.) On the copy I change the namespace to match my new assembly.

  namespace LoggingUtilities

  {

      public class Logger

      {

          public void LogError(string message)

          { 

              Debug.WriteLine("Error: " + message);

          }

      }

  }

In my original Logger class, I create an instance of my new Logger. Each method in the original class now forwards requests to the new Logger instance. In this way, I am wrapping the new class in the original. This allows applications to still use the old class, though the functionality has been moved.

  public class Logger

  {

      LoggingUtilities.Logger _logger =

          new LoggingUtilities.Logger();

      [Obsolete("Use LoggingUtilities.Logger instead")]

      public void LogError(string message)

      {

          _logger.LogError(message);

      }

  }

As before we need to evaluate referencing projects. Because our original class still exists, these applications will continue to compile.

Note that I've added an "Obsolete" attribute to the LogError method. This means we will receive a compiler warning (or error) that we need to change our application to use the new class. This makes it clear what needs to be modified, saving time on any rework.