Dependency Breaking Techniques: Inline Static Class

Often times I run into a class that has a dependency not on a Singleton, but a static class.  When refactoring away from a Singleton, a common approach is to use Inline Singleton.  With static classes, a slightly different approach needs to be taken because client code isn’t working with an instance of a type, but rather static methods on the type itself.

Dependency breaking techniques are used to get legacy code under test.  The goal isn’t necessarily to break all client dependencies out, but just the one I’m modifying to get under test.  I’m not interested changing any of the other clients that may use this static class.  Since nothing is under test, it’s too risky to make non-backwards compatible changes.

The pattern

Code needs access to a method or property but doesn’t need a global point of access to it.

**Move Static’s features to instance methods of that type.
**

** **

Motivation

Helper methods tend to congregate into static classes with static methods.  Over time, the responsibility grows for the static class until it becomes a dumping ground to any behavior that seems relevant to the name.  The name of the static class usually is post-fixed by “Helper”, “Manager”, “Utility”, or other generic and obtuse names.

Eventually, this static class will have opaque dependencies of its own, where callers don’t know what’s happening behind the scenes when those helper methods are called.  This can wreak havoc with unit testing, especially when trying to add tests to legacy code.

Mechanics

1. Find the static method in the calling class and use Extract Interface to extract an interface that contains only the static method being called in the client code.
2. Make the static class explicitly implement the new extracted interface.  The static class should have two methods with identical signatures, one a static method, one an explicitly implemented interface method.
3. Use Move Method to move the logic from the static method to the instance method.
4. Use Hide Delegate to delegate the static method calls to the use the instance method instead.
5. Compile and test.
6. Modify Client code using the static Operation method to instantiate an IStatic instance and call the interface Operation method.
7. Compile and test.
8. Use Extract Parameter to pass in the IStatic instance to the Client method.  If IStatic is a primal dependency, extract the parameter to a constructor argument and set the variable to a local field.
9. Compile and test.

Example

In our e-commerce system, one of the pages the user is presented with is a payments page.  It is in this page that the user decides how they want to pay for their order, whether it’s credit card, invoice, financing, etc.

Not all payment types should be displayed for all carts, and there is some complex business logic that determines who sees what payment types.  There are several payment filtering strategies to encapsulate this business logic, and one type uses the user’s profile to filter the payments.  Here’s the strategy class:

public class AccountFilter
{
    private readonly string[] _paymentTypes;

    public AccountFilter(string[] paymentTypes)
    {
        _paymentTypes = paymentTypes;
    }

    public void AddPaymentOptions(ShoppingCart cart)
    {
        IPayment payment = ProfileManager.FindPaymentByType(_paymentTypes);

        if ((payment != null) &&
            (! string.IsNullOrEmpty(cart.AccountNumber)))
        {
            cart.PaymentFilters.Add(payment);
        }
    }

}

The business has a new requirement for us: Payment types should not be filtered using the user’s profile for existing quotes.  In our system, customers can save their cart as a “quote”, so that the unit cost information can be “locked” for a set amount of time.  Enabling this change requires us to change the AccountFilter class.

Being a legacy code system, the AccountFilter class has no tests defined on it.  It’s our goal to make the change and add tests to verify the new requirements, and that’s all.  We’re not trying to add 100% unit test coverage for the AccountFilter class, just to test the changes we’re making.

Not knowing if I can unit test this class, the easiest way to see if it’s testable is to try using it in a test.  Here’s the behavior I want, in a unit test:

[TestMethod]
public void Should_not_add_filter_when_basket_is_a_quote()
{
    ShoppingCart cart = new ShoppingCart();
    cart.IsQuote = true;
    cart.AccountNumber = "123ABCD";

    AccountFilter filter = new AccountFilter(new string[] { "CRD", "INV" });
    filter.AddPaymentOptions(cart);

    Assert.AreEqual(0, cart.PaymentFilters.Count);
}

I try running this test, and it fails spectacularly.  The error I get is along the lines of “HttpContext.Current is null”, so something is dependent on the ASP.NET runtime being up.

Looking at the AddPaymentOptions method of the AccountFilter class, I notice the call to the static method FindPaymentByType on the ProfileManager class.  That method is extremely hideous, with hard dependencies on HttpContext.Current, a web service, and a database.  Instead of focusing on breaking those dependencies out, I can apply Inline Static Class and break out AddPaymentOptions’ dependency on ProfileManager.  The end goal is to break the dependencies of the class I’m trying to test, not all the sub-sub-sub dependencies that may be around.

Step 1:

ProfileManager is a huge class, with about 100 different methods.  The only one I’m interested in is the FindPaymentByType method, so I’ll create an IProfileManager interface that includes only that method, matching signature and name.

public interface IProfileManager
{
    IPayment FindPaymentByType(string[] paymentTypes);
}

Step 2:

Now that I have my IProfileManager, I’ll make the static class implement the new interface.  Note that the static class itself isn’t marked “static”, only its methods.  Additionally, I’ll need to explicitly implement that method so it doesn’t conflict with the existing static method.

public class ProfileManager : IProfileManager
{
    public static IPayment FindPaymentByType(string[] paymentTypes)
    {
        // Hit web service for widget
        // Query database based on widget
        // Return database object
    }

    IPayment IProfileManager.FindPaymentByType(string[] paymentTypes)
    {
        return null;
    }
}

I compile, and everything looks good.  Notice I didn’t add any access modifiers to the new IProfileManager method, and the inclusion of the interface name prefixed on the method name.  That’s how I can explicitly implement the interface, and not get any compile errors even though these two methods have the exact same signature.

Step 3, 4:

Now I can move the existing implementation to the new interface method, and use Hide Delegate to change the static method to call the new interface method.  I don’t want to introduce duplication and simply copy over the implementation, so Hide Delegate allows me to retain backwards compatibility and save a lot of work of changing all clients to use the new method.

public class ProfileManager : IProfileManager
{
    public static IPayment FindPaymentByType(string[] paymentTypes)
    {
        IProfileManager profileManager = new ProfileManager();
        return profileManager.FindPaymentByType(paymentTypes);
    }

    IPayment IProfileManager.FindPaymentByType(string[] paymentTypes)
    {
        // Hit web service for widget
        // Query database based on widget
        // Return database object
    }
}

Now my static method delegates to the instance methods, and no other clients are affected by this change.  Again, it’s not my goal to fix every client dependency on this static method, only the one I’m changing.

I compile and test, but my initial unit test is failing as it’s still calling the static method.

Step 6:

Now that I have an interface to depend upon instead of a static method, I’ll change the client code to use the interface instead of the static method.  It looks much like the code inside the Hide Delegate of the static method:

public void AddPaymentOptions(ShoppingCart cart)
{
    IProfileManager profileManager = new ProfileManager();
    IPayment payment = profileManager.FindPaymentByType(_paymentTypes);

    if ((payment != null) &&
        (! string.IsNullOrEmpty(cart.AccountNumber)))
    {
        cart.PaymentFilters.Add(payment);
    }
}

I create a variable of type IProfileManager, instantiate it to a ProfileManager instance, and use the IProfileManager.FindPaymentByType method instead of the static method.  I compile and test, but my unit test still fails as I still haven’t fully extracted the dependency on the ProfileManager, but that will be taken care of shortly.

Step 8:

I have two options, I can either pass in the IProfileManager as in the method call or I can pass it in through the constructor.  I prefer to pass dependencies through constructors over method parameters, because I like a clear separation between queries, commands, and dependencies.  Additionally, if I’m using an IoC container, I can wire up these dependencies a little more easily.  I’ll use Preserve Signatures [Feathers 312] and Chain Constructors to keep the original constructor, as there are still quite a few clients of my AccountFilter class that I don’t want to change:

public class AccountFilter
{
    private readonly string[] _paymentTypes;
    private readonly IProfileManager _profileManager;

    public AccountFilter(string[] paymentTypes)
        : this(paymentTypes, new ProfileManager()) { }

    public AccountFilter(string[] paymentTypes, IProfileManager profileManager)
    {
        _paymentTypes = paymentTypes;
        _profileManager = profileManager;
    }

    public void AddPaymentOptions(ShoppingCart cart)
    {
        IPayment payment = _profileManager.FindPaymentByType(_paymentTypes);

        if ((payment != null) &&
            (!string.IsNullOrEmpty(cart.AccountNumber)))
        {
            cart.PaymentFilters.Add(payment);
        }
    }
}

So I did a few things here:

• Add a new constructor that took an IProfileManager, and created the backing field for it
• Make the other constructor call the new one, passing in a new ProfileManager, which is what existing code would do
• Make the AddPaymentOptions method use the field variable instead of a local variable

What this code allows me to do is pass in a different IProfileManager instance in my test methods to verify my behavior.  I don’t really care what the ProfileManager does, all I care about is what it tells me.  Somewhere down the line, I have an integration test that puts the two together, but until then, my unit test will suffice.  Here’s my final unit test, which fails for the reason I want it to fail, because the assertion fails:

[TestMethod]
public void Should_not_add_filter_when_basket_is_a_quote()
{
    ShoppingCart cart = new ShoppingCart();
    cart.IsQuote = true;
    cart.AccountNumber = "123ABCD";

    string[] paymentTypes = new string[] { "CRD", "INV" };

    MockRepository repo = new MockRepository();

    IProfileManager profileManager = repo.CreateMock<IProfileManager>();

    using (repo.Record())
    {
        Expect.Call(profileManager.FindPaymentByType(paymentTypes))
            .Return(null);
    }

    using (repo.Playback())
    {
        AccountFilter filter = new AccountFilter(paymentTypes, profileManager);
        filter.AddPaymentOptions(cart);
    }

    Assert.AreEqual(0, cart.PaymentFilters.Count);
}

I use RhinoMocks to pass in a mock IProfileManager, and test-driven development to add the functionality desired.  I broke the dependency of the class under test away from the ProfileManager static method, and was able to preserve existing functionality.  By preserving existing interfaces and functionality, I can eliminate much of the risk involved in changing legacy code.  I got my changes in, was able to test them, and kept out of the rabbit-hole that a larger refactoring would have thrown me into.