Unit testing legacy code with static classes

Question

I am adding new functionality into a legacy application. For my new classes I have introduced Unit Testing. I still need to work with the existing code.

One of the problems I face is that a lot of static methods have been used.

Below is an example of a class I need to use:

public class HelperClass
{
    public static bool ReturnSomething()
    {
        return true;
    }
}

My fix for making this testable:

public interface IHelperClass
{
    bool ReturnSomething();
}

public class HelperClass : IHelperClass
{
    public static bool ReturnSomething()
    {
        return true;
    }

    bool IHelperClass.ReturnSomething()
    {
        return ReturnSomething();
    }
}

A class that used to use the static method, but by introducing a property (initialized by the constructor), that now uses the testable class. I can inject a mock using this property. Note that I could have used a factory for injecting the mock, however I wanted to keep it simple for the example.

public class UserClass
{
    public UserClass()
    {
        // Initialize with default 
        HelperClass = new HelperClass();
    }

    public IHelperClass HelperClass { get; set; }
}

My advantages:

• No need to modify existing code that is using the static class
• Code can be migrated from static to a testable instance
• I can reuse the existing concrete class

My disadvantages:

• Needed to modify legacy code that needs to be tested. Not always possible when in external assembly.
• PropertyName is the same as the class Name

I welcome any feedback!

Answer 1

I happen to like your fix for making it testable, but would reverse the implementation order between the static method and the explicit interface method. This will allow you to slowly change existing code first to use the Singleton instead of the static and then finally use an injectable pattern:

public interface IHelperClass
{
    bool ReturnSomething();
}

public sealed class HelperClass : IHelperClass
{
    private static readonly IHelperClass instance = new HelperClass();

    private HelperClass()
    {
    }

    public static IHelperClass Instance
    {
        get
        {
            return instance;
        }
    }

    [Obsolete("Use the Instance singleton's implementation instead of this static method.")]
    public static bool ReturnSomething()
    {
        return Instance.ReturnSomething(); // Simply calls the Singleton's instance of the method.
    }

    bool IHelperClass.ReturnSomething()
    {
        return true; // The original static ReturnSomething logic now goes here.
    }
}

Answer 2

What about creating new classes that use the existing static methods rather than extending existing code. That way you will not need to worry about breaking existing code or adding duplicate method calls onto existing?

One thing I would say for certain is to stay away from a property name being the same as the class name. Name it something different at least. Even just Helper is better than HelperClass (although probably not much).

Something alot along the lines you mentioned but very slight differences:

public interface IHelper
{
   bool ReturnSomething();
}

public class HelperAdapter : IHelper
{
   bool IHelperClass.ReturnSomething()
   {
      return HelperClass.ReturnSomething();
   }
}

And it is used like:

public class UserClass
{
   private readonly IHelper _helperAdapter;

   public UserClass()
      : this(new HelperAdapter())

   public UserClass(IHelper helper)
   {
       helperAdapter = helper;
   }
}

Answer 3

Personally I've found explicitly implementing interfaces to be a world of hurt with difficult to track down bugs and odd debug views.

The Simplest solution (and the best one IMO when the method doesn't access or modify state):

public class HelperClass
{
    internal static Action<bool> returnSomethingImpl = () => {
      return true;
    }
    public static bool ReturnSomething() { return returnSomethingImpl(); }
}

then in your AssemblyInfo.cs

[assembly:InternalsVisibleTo('MyTestDllNameWithoutTheDll')]

and just stub it out in your test however you want:

HelperClass.returnSomethingImpl = () => {
  calls.Add("returnSomethingImpl called");
  return false;
}

If you ARE reading/writing state then a singleton would probably serve best

public class HelperClass : IHelperClass
{
    IHelperClass _instance;
    public IHelperClass Instance 
    {
      get {
        if(_instance == null) Instance = new HelperClass();
        return _instance;
      } 
      set { _instance = value }
    }    
    public static bool ReturnSomething() { return Instance.Value.ReturnSomething(); }
}

The only option that will allow you not to change ANY of the existing code at all is to use a super-powered mocking framework like Typemock Isolator (or Moles) which can mock out just about anything, including statics.