Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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!

share|improve this question

3 Answers 3

up vote 8 down vote accepted

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.
    }
}
share|improve this answer
    
Thank you! That was indeed one of my requirements; to be able to slowly change existing code (as a lot depends on it). I have already applied my refactoring, but I will note this solution for future problems! –  Maurice Stam Mar 28 '12 at 7:14
3  
I like the use of the Obsolete attribute in this situation. –  dreza Mar 28 '12 at 19:05

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;
   }
}
share|improve this answer
    
I agree with you that using the same name is bad. Actually it made converting existing code easier, however for readability it is indeed better to use the 'adapter' naming convention. –  Maurice Stam Mar 27 '12 at 14:55
    
I think I like this solution best. It minimizes the change required to abstract the static, and it allows you to inject a mock helper when necessary. –  Sean H Mar 28 '12 at 14:10

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.

share|improve this answer
    
Unfortunately i can not use Typemock. We do use the Moq framework. The static class is indeed reading / writing state. Therefore it should have been implemented as a singleton. Great review. –  Maurice Stam Mar 27 '12 at 15:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.