Friday, May 14, 2010

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.