Refactoring Legacy Code - Part 1: Dealing with Static Cling

Introduction

Refactoring legacy code is always a challenge. As per Working effectively with Legacy Code, by Michael Feathers, 'Legacy code is any code without unit tests'. To refactor, you need unit tests. To add unit tests to legacy code which was not written with testability in mind, we need to refactor it. It becomes a chicken and egg story, and in the end, neither refactoring nor adding unit tests happen. Legacy code continues to deteriorate, and becomes unmanageable eventually.

Some of the common challenges we run into in 'refactoring legacy code' include (but not limited to):

• Mocking static methods
• Mocking singletons
• Mocking static factory methods
• Breaking dependencies

In this series of articles, we will see some techniques and recipes from experts on 'refactoring legacy code', with relevant examples. Part 1 of this article concentrates on 'Static Cling'. A small disclaimer: Experts who deal with legacy code on a daily basis and TDD wonks will find most of the article a brain dump, what they do unconsciously as a set of common sense practices. Yet, to starters who are afraid to take the plunge, I believe this series will serve as a starting point.

Static Cling

Static cling is caused by static electricity, usually due to rubbing, as in a clothes dryer. That's the actual static cling. In 'Working effectively with Legacy Code', Michael Feathers defines static cling as 'a static member which contains something that is difficult to depend on in a test'. We will look into a few methods of eliminating static cling in a phased manner.

Static methods are a pain when we try to get legacy code under the safety net of unit tests. Static methods are a pain because they are difficult to mock in a unit test. If we do not mock a 'static cling', we are not really writing a unit test, but an integration test. It is not that integration tests are bad, but we need unit tests in addition to the integration tests. There are mocking frameworks like TypeMock which let you mock static code. But most of the times, 'static cling' in legacy code is procedural code done in an object oriented language like C#. We can use sophisticated tools and apply deodorant on the smell, or we should refactor the code to true OOP.

Legacy Code Change Algorithm

When working with legacy code, it is very easy to get carried away and make a big bang refactoring (actually, 'code cleanup and pray' is a better term since we do not have any tests). In this case, we can try to eliminate the static cling by making the static method non static, and extracting the non-static method into an interface. Make the clients (all the call sites) use the interface. This would mean that we deal with potentially a few changes to the client code to a few hundreds in some worst case scenarios, all at one time. The result could be 'Object Reference not set to an instance of an object' - Anonymous. We want to be able to make this change in a phased manner in small manageable increments.

Approach

1. Pick one call site.
2. Change in one call site.
3. Bring it under the safety net of unit tests.
4. Make sure the change works.
5. Meanwhile, the other call sites continue using the original unchanged code and continue to work fine.
6. Go back to step 1 till all the call sites are refactored, and remove the static method finally.

Let us see how these steps map to one of the techniques in eliminating static cling, 'Introduce instance delegator - p369, Working Effectively with Legacy Code'.

Introduce Instance Delegator

1. If the class which contains the static method is static (.NET Framework 2.0+), make it non static.
2. Introduce an instance method with the same signature which delegates to the static method.
3. Use ExtractInterface and extract the instance method created.
4. At the call site for the static method, change to use the instance method (called through the extracted interface) instead of the static method.
5. The instance can be provided at the call site:
   • as a parameter
   • via setter injection
   • via constructor injection

Most importantly, the other call sites continue using the static method till we get them under the safety net of unit tests. Enough said, let us see some code.

public static class AuthorizeHelper {
    //...
    public static List<UserProfile> GetCachedProfile(IToken token)     {
        ISecurityCacheProvider securityCacheProvider =
          SecurityProviderFactory.GetSecurityProviderFactory().GetSecurityCacheProvider();
        List<UserProfile> userProfiles = 
          (List<UserProfile>)securityCacheProvider.GetProfile(token);
        return userProfiles;
    }
    //...
}

// ...
// Call site
public static QueryItem GetQueryItem(IToken token, int objectTypeID,
    int? childObjectTypeID,
    QueryItem sanatizedQuery, RelatedAccessType relatedAccessType,
        RequestType requestType) {
    List<UserProfile> userProfiles = AuthorizeHelper.GetCachedProfile(token);
    // ...
}
// ...

/*
                ||        
                ||
                \/

*/

public class AuthorizeHelper : IAuthorizeHelper {
    //...
    public static List<UserProfile> GetCachedProfile(IToken token)     {
        ISecurityCacheProvider securityCacheProvider =
          SecurityProviderFactory.GetSecurityProviderFactory().GetSecurityCacheProvider();
        List<UserProfile> userProfiles = 
           (List<UserProfile>)securityCacheProvider.GetProfile(token);
        return userProfiles;
    }
    public List<UserProfile> GetProfileFrom(IToken token) {
        return GetCachedProfile(token);
    }
    //...
}

public interface IAuthorizeHelper {
    List<UserProfile> GetProfileFrom(IToken token);
}

// ...
// Call site when using a parameter to provide the instance
// for new clients & the test which we want to write
public QueryItem GetQueryItem(IToken token, int objectTypeID, int? childObjectTypeID,
    QueryItem rawQuery, RelatedAccessType relatedAccessType, RequestType requestType, 
                        IAuthorizeHelper authorizeHelper) {
    List<UserProfile> userProfiles = authorizeHelper.GetProfileFrom(token);
    // ...
}

// for use of existing clients which we do not want to change
public QueryItem GetQueryItem(IToken token, int objectTypeID, int? childObjectTypeID,
    QueryItem rawQuery, RelatedAccessType relatedAccessType, RequestType requestType) {
    IAuthorizeHelper authorizeHelper = new AuthorizeHelper();
    return GetQueryItem(token, objectTypeID, childObjectTypeID, rawQuery, 
                        relatedAccessType, requestType, authorizeHelper);
}

// ...
// Call site when using setter injection to provide the instance
public QueryItem GetQueryItem(IToken token, int objectTypeID, int? childObjectTypeID,
    QueryItem rawQuery, RelatedAccessType relatedAccessType, RequestType requestType) {
    List<UserProfile> userProfiles = authorizeHelper.GetProfileFrom(token);
    // ...
}
private IAuthorizeHelper authorizeHelper;
public IAuthorizeHelper AuthorizeHelper {
    get {    
        // for use of existing clients which we do not want to change        
        if(authorizeHelper == null) { 
            authorizeHelper = new AuthorizeHelper();
        }
    return authorizeHelper;
    }
    set {
        authorizeHelper = value;
        // for new clients & the test which we want to write
    }
}

// ...
// ...
// Call site when using constructor injection to provide the instance
public QueryItem GetQueryItem(IToken token, int objectTypeID, int? childObjectTypeID,
       QueryItem rawQuery, RelatedAccessType relatedAccessType, 
       RequestType requestType) {
    List<UserProfile> userProfiles = 
                authorizeHelper.GetCachedProfile(token);
    // ...
}
private IAuthorizeHelper authorizeHelper;
public SecurityQueryBuilder() : this(new AuthorizeHelper()) { }
// for use of existing clients which we do not want to change
public SecurityQueryBuilder(IAuthorizeHelper authorizeHelper) { 
    // for new clients & the test which we want to write
    this.authorizeHelper = authorizeHelper;
}
// ...

As the code evolves, we will get to a point where every call to the utility class comes through the delegating methods. At that time, we can move the implementations of the static methods into the instance methods and we can delegate the static methods.

Variation

(Can be used when there are only a few call sites.)

In some cases, we see that the legacy code has static methods with parameters as objects and which use public states from parameter classes. Find the parameter object whose state the static method is predominantly using (Feature Envy on). Consider moving the static method into the parameter object's class. The procedural code gets information and makes decisions, the object oriented code tells the other object what to do — tell do, not ask. Resharper has a Make method non-static refactoring which works this way. But exercise caution, and make sure you analyze usages before making this refactoring.

Let us see an example.

public class AuthorizeHelper {
    //...
    public static bool HasDeepAccess(RolePrivilege rolePrivilege) {
            if (rolePrivilege.IsDeep != Permission.NotSet && 
                rolePrivilege.IsDeep == Permission.Grant)
                  return true;
            else
                  return false;
        }
    //...
}
public class RolePrivilege {
    //...
    public Permission IsDeep {get; private set; }
    //...
}
/*
                ||
                ||
                \/
*/
public class RolePrivilege {
    private Permission isDeep;
    public bool HasDeepAccess() {
        if (isDeep != Permission.NotSet && isDeep == Permission.Grant)
                    return true;
                else
            return false;
    }
}

The IsDeep property was reduced to a private attribute on RolePrivilege in the process, which is good old OO principle compliant (encapsulation).

Introduce Static Setter

Sometimes, creational pattern madness results in a static cling. Let us see the steps in eliminating static cling involving object creation, 'Introduce Static Setter - p369, Working Effectively with Legacy Code'

1. Make the constructor protected if it is private / class non-static (.NET 2.0+) if it is static.
2. Add a static setter to the singleton class.
3. If we need access to private state / behavior in the singleton to set it up properly for testing, consider making them protected.

public class InProcCacheStore : ICacheStore {
    #region Singleton Implementation
    private IDictionary<string,> backingstore;
    private static InProcCacheStore _instance;
    private static object lockobject = new object();
    private InProccacheStore()  (
        _backingStore = new Dictionary<string,>();
    }
    public static InProcCacheStore Instance {
        get {
            if(_instance == null) {
                lock(lockObject) {
                    if(_instance == null)
                        _instance = new InProcCacheStore();
                }
            }
            return _instance;
        }
    }
    #endregion

    // ICacheStore Implementation
}
/*
                ||
                ||
                \/
*/
public class InProcCacheStore : ICacheStore {
    #region Singleton Implementation
    protected IDictionary<string,> backingstore;
    private static InProcCacheStore _instance;
    private static object lockobject = new object();
    protected InProccacheStore()  (
        _backingStore = new Dictionary<string,>();
    }
    public static InProcCacheStore Instance {
        get {
            if(_instance == null) {
                lock(lockObject) {
                    if(_instance == null)
                        _instance = new InProcCacheStore();
                }
            }
            return _instance;
        }
    }
    #endregion
    
    #region Test Specific Implementation
    public static InProcCacheStore TestInstance {
        set {
            _instance = value;
        }
    }
    #endregion

    // ICacheStore Implementation
}
public class TestCacheStore : InProcCacheStore { //... }

Introduce Static Setter — Variation for Static Factory Methods

Another pattern which we come across often in legacy code is static Factory methods. This is the variation of 'Introduce Static Setter' for static factory methods.

public class SecurityProviderFactory {
    public static ISecurityProvider GetSecurityProvider()
        // TODO: Read the security provider from config
        return new DbsecurityProvider();
    }
}
                ||
                ||
                \/
public interface ISecurityProviderFactory {
    ISecurityProvider CreateSecurityProvider();
}
public class SecurityProviderFactory : ISecurityProviderFactory {
    private static ISecurityProviderFactory providerFactory;
    public static ISecurityProvider GetSecurityProvider() {
        if(providerFactory == null)
            providerFactory = new SecurityProviderFactory();
        return providerFactory.createsecurityProvider();
    }
    public ISecurityProvider CreateSecurityProvider() {
        // TODO: Read the security provider from config
        return new DbsecurityProvider();
    }
    public ISecurityProviderFactory TestSecurityProviderFactory {
        set {
            providerFactory = value;
        }
    }
}

Another simple refactoring which will help reduce object creation headaches and make it possible to inject mocks is the good old 'Single Responsibility Principle'. A method should not create an object as well as use it (it becomes two responsibilities then).

public class EntityPresenter {
    private IEntityView view;
    public EntityPresenter(IEntityView view) {
        this.view = view;
    }
    public void LoadEntity() {
        IEntityservice service = ServiceFactory<ientityservice>.CreateService();
        Entity entity = service.LoadEntity(view.EntityId);
        view.Bind(entity);
    }
}
                ||
                ||
                \/
public class EntityPresenter {
    private IEntityView view;
    private IEntityService service;
    // for use of new cients & test code
    public EntityPresenter(IEntityView view, IEntityService service) {
        this.view = view;
        this.service = service;
    }
    // for use of existing cients
    public EntityPresenter(IEntityView view) :
        this(view, ServiceFactory<ientityservice>.CreateService()) {
        public void LoadEntity() {
            Entity entity = service.LoadEntity(view.EntityId);
            view.Bind(entity);
        }
    }
}

Summary

In this article, we tried to address static cling using these options:

1. Introduce instance delegator
2. Move static method on one of the parameter objects and make method non-static
3. Introduce static setter
4. Separate creation from use

Stay tuned for the next article in the series. Till then, develop with passion!

Reference

• Working Effectively with Legacy Code by Michael C. Feathers