Refactoring to Improve Testability: Removing Unnecessary Dependencies

Nowadays I am trying to stick to TDD (with the test first approach) and have found it to be of great help. One of the biggest reward doing TDD is that it helps me to stay in the flow and regain speed faster after a distraction. This post explains how to refactor code to remove unnecessary dependencies, which is easily found when writing tests.

Unnecessary dependencies are those components which a SUT depends on, but does not directly affect any of its functionalities. Some of the common tests smell (from XUnit Test Patterns by Gerard Meszaros, a recommended read) that helps me to find these dependencies are Test Code Duplication and Fragile Tests.

Cut-and-Paste code reuse for fixture setup happens often when there is an unnecessary dependency.

While looking for an example to write on, I came across a post from my friend Bappi, where he explains Testing Codes with ConfigurationManager. It's a good read on how to remove the dependency with various Configuration Providers by creating an abstraction over it.

Testability Issues with Current Design

While abstracting the Configuration Manager by using an interface is a good idea, you should also be careful on how the application classes depend on it. Configurations live at the application root and it is a good idea to restrict dependencies with it at that level. Rest of the application must be dependent only on the configuration value and not the configuration itself. Inner components having dependency with the configuration provider brings in unnecessary complexities and makes code fragile. Some common issues are

• Class needs to know of Configuration key
• Extra mocking while testing

As you see below, the test case from the original post has to set up the Configuration provider mock to return values before testing the class. MyService (assuming that it is not a Factory class, which I confirmed from Bappi) is unnecessarily depending on IAppSettings and coupling itself with the configuration name, which really is not its concern. This leads to brittle code and tests!

[Subject(typeof(MyService))]
public class MyServiceTests
{
    Establish context = () =>
        {
            otherDependency = Substitute.For<IMyOtherDependency>();

            var appSettings = Substitute.For<IAppSettings>();
            appSettings["app.name"].Returns("My Test Application");

            myService = new MyService(otherDependency, appSettings);
        };

    Because of = () => result = myService.PerformOperations();

    It should_call_my_dependency_utility_method_once = () => otherDependency.Received(1).UtilityMethod();
    It should_execute_successfully = () => result.ShouldBeTrue();
}

Refactoring the Code

Refactoring such code is as easy as removing the dependency on IAppSettings and taking in the value of 'app.name' as the dependency. This removes the interface dependency and requires only the string value to be passed in. Here I am passing in an anonymous Name, as the value is not of concern for this test.

[Subject(typeof(MyService))]
public class MyServiceTests
{
    Establish context = () =>
        {
            otherDependency = Substitute.For<IMyOtherDependency>();
            var anonymousName = "Anonymous Name";
            myService = new MyService(otherDependency, anonymousName);
        };

    Because of = () => result = myService.PerformOperations();

    It should_call_my_dependency_utility_method_once = () => otherDependency.Received(1).UtilityMethod();
    It should_execute_successfully = () => result.ShouldBeTrue();
}

When looked at isolation these are minor code changes that hardly removes a line or two. But it has a cumulative effect when applied to all the tests for the class and makes code more robust.

When looked at isolation, this is a seemingly minor change of not mocking an interface and is just one line of code, which you could live with. But you need to mock that for all tests of that class, which is when you start to see the real benefit. Also, you have made the tests more resilient by not taking an unnecessary dependency. Even if you decide to change the configuration name to 'ApplicationName', none of the tests break now, whereas with the original code all of them would have.

One possible argument with this refactoring is, ** What if I need an extra value from the dependency (app.domain in the above case), I now have to update the class constructor**.

Agreed, but then this violates Open Closed Principle, which states 'You should be able to extend a classes behaviour without modifying it.' If you need a new configuration value, you are essentially changing the components functionality, so you should either extend current functionality or write a new component. This also opens up a hidden code smell with the existing code and an anti-pattern - Service Locator. So the refactoring still holds good!

Hope this helps you find dependencies with unnecessary components and remove them.