Dealing With Legacy Code When Using TDD

Introduction

You may have heard about TDD lately and how great it is and so on, but most the time, we work on projects that have a large code base and we are not allowed to make drastic changes.

Tools

• NUnit

Scenarios

In my experience, some of the scenarios we face when refactoring legacy code are:

• We are free to apply the changes we want, the world is perfect. :)
• We can apply the changes we want but only with unit testing purposes, production code should work as usual.
• We are not allowed to change any part of the code except infrastructure but remembering that things must work as always did, no breaking changes allowed.

Let’s elaborate on each one of those scenarios and how we perform our changes to make legacy code more testable.

Concrete Example

public static class DataBaseHelper
{
    public static void Persist(object instance)
    {
        Console.WriteLine("Persisting object " + instance);
    }
}

public class ProductService
{
    public void DisableProduct(Product product)
    {
        product.Enabled = false;
        DataBaseHelper.Persist(product);
    }
}

As you can see, the Persist method from the DataBaseHelper does nothing, but we will assume we’ve some hard dependency with a database, perhaps an sqlconnection here, an sqlcommand there and so on.

Now, what’s so bad with this piece of code? Most of us have seen, done and maintained stuff like that for years and if you’re fine with this, go ahead, I’m not here to tell you what’s wrong and what’s perfect, there’re no perfect solutions. However, I would like to show you another way, a better way in my opinion.

I asked what was bad with that piece of code and I’ve not answered that yet…Let’s see:

• Can you be really sure that in the following days/weeks/months/years that code will work as expected?
• If in the future you found a bug in the system, how would you discard the problem cause is not related to this method? And when checking this fact, do you need to have your debugger/Visual Studio instance up and running?

I suppose that the answers weren't so pleasant at all, but don’t worry, the idea of this article is to help you to find a sane path when developing software by using TDD.

What Should Our Tests Cover

Well, there are a couple of things we should ensure in our tests:

1. The product passed to the service method enters with its Enabled property with a True value, and must exit with a value of False.
2. The service should persist the updated product instance.

Now let's back to the Scenarios section and supply a workaround for every one of those points.

We are free to apply the changes we want, the world is perfect. :)

This allows us to use a fantastic set of patterns, I’m talking about Inversion Of Control and Dependency Injection.

By using that, we can make the following changes:

public class ProductService
{
    private readonly IProductRepository _productRepository;

    public ProductService(IProductRepository productRepository)
    {
        _productRepository = productRepository;
    }

    public void DisableProduct(Product product)
    {
        product.Enabled = false;
        _productRepository.Persist(product);
    }
}

public interface IProductRepository
{
    void Persist(Product product);
}

public class ProductRepository : IProductRepository
{
    public void Persist(Product product)
    {
        DataBaseHelper.Persist(product);
    }
}

public static class DataBaseHelper
{
    public static void Persist(object instance)
    {
        Console.WriteLine("Persisting object " + instance);
    }
}

And introduce the following tests:

public class Scenario1Tests
{
    #region Mocks
    private class NoOpProductRepository : IProductRepository
    {
        public void Persist(Product product)
        {
            // no-op
        }
    }

    private class VerifiableProductRepository : IProductRepository
    {
        public event Action<Product> ProductPersisted;
        public void Persist(Product product)
        {
            var productPersistedEvent = ProductPersisted;
            if (productPersistedEvent != null)
                productPersistedEvent(product);
        }
    }

    #endregion


    [Test]
    public void when_disabling_a_product_its_enabled_property_is_set_to_false()
    {
        // Arrange
        var product = new Product
                          {
                              ProductId = 1,
                              ProductName = "Car",
                              Enabled = true
                          };
        // here we should use a mocking framework like Moq or Rhino.Mocks.
        IProductRepository mockedRepository = new NoOpProductRepository();

        var service = new ProductService(mockedRepository);

        // Act
        service.DisableProduct(product);

        // Assert
        Assert.AreEqual(false, product.Enabled);
    }

    [Test]
    public void when_disabling_a_product_it_persist_the_changes_using_the_product_repository()
    {
        // Arrange
        var product = new Product
        {
            ProductId = 1,
            ProductName = "Car",
            Enabled = true
        };
        Product productPassedToPersistMethod = null;
        var valueOfProductEnabledPropertyPassedToPersistMethod = false;
        // here we should use a mocking framework like Moq or Rhino.Mocks.
        var mockedRepository = new VerifiableProductRepository();
        mockedRepository.ProductPersisted +=
            instance =>
                {
                    productPassedToPersistMethod = instance;
                    valueOfProductEnabledPropertyPassedToPersistMethod = instance.Enabled;
                };

        var service = new ProductService(mockedRepository);

        // Act
        service.DisableProduct(product);

        // Assert
        Assert.IsNotNull(productPassedToPersistMethod);
        Assert.AreSame(product, productPassedToPersistMethod);
        Assert.AreEqual(false, valueOfProductEnabledPropertyPassedToPersistMethod);
    }
}

As you can see, those are quite drastic changes, in order to use again our ProductService, now we have to supply an IProductRepository implementation because its constructor demands one. The usage of the static helper class to access to the database it has been moved our concrete implementation of the ProductRepository which is likely to be used at runtime. This is where the usage of a IoC/DI tool like StructureMap really shines because it will help us to configure such concerns (i.e., connect IProductRepository with ProductRepository) and make it transparent its usage.

Now what do we gain by doing this? Well, now we can test the ProductService.DisableProduct method at our pleasure by mocking its dependencies and ensuring everything works as expected. If someone in the future tries to mess with the implementation of such method or if we’ve to introduce changes into our system, we’ll have those two unit tests to cover our backs and they’ll let us sleep tight at night. I think you can figure out what the tests do by just looking at the code, it is fairly simple and it does not take more than 10 minutes to create them.

We can apply the changes we want but only with Unit Testing purposes, production code should work as usual.

Here, we can use something called Poor Man’s Dependency Injection, you can read about it here, and let me tell you something, I agree with the thoughts Jimmy Bogard gives us in that article. And I really think dear developer, we should strive all the time for option number one, push harder to let management change the infrastructure to use proper architecture and a DI/IoC tool. However, the world is not perfect and management or our work environment isn’t always too kind to accept changes.

So what we do? We take the code from the first approach and modify the ProductService class and introduce a parameterless constructor which calls to the constructor that takes in an IProductRepository instance and pass to it an instance of the ProductRepository class, thus, removing the need of a DI/IoC tool to build the instances in our regards.

Here is the code to illustrate what I was mumbling in the previous paragraph:

public class ProductService
{
    private readonly IProductRepository _productRepository;

    public ProductService()
        : this(new ProductRepository())
    {

    }

    public ProductService(IProductRepository productRepository)
    {
        _productRepository = productRepository;
    }

    public void DisableProduct(Product product)
    {
        product.Enabled = false;
        _productRepository.Persist(product);
    }
}

The tests and all the other stuff we added remain the same, we still have a testable ProductService class.

We are not allowed to change any part of the code except infrastructure but remembering that things must work as they always did, no breaking changes allowed.

This means, no interfaces, no repositories, nothing, thus, no new concepts are allowed to be introduced into the principal code base, you should keep your changes as low as possible.

Well, I must reckon that does not let us with much choices to pick from…but even in such an inflexible environment, we can do better with simple approaches.

What we will do is to take our helper and make it flexible enough to allow us to check the ProductService will work as expected in the future. Certainly, this technique isn’t mine, at the moment, I can find the resource where I read about it for the first time, so, if someone has a link, be my guest.

public static class DataBaseHelper
{
    private static Action<object> _persistenceDelegate;

    static DataBaseHelper()
    {
        _persistenceDelegate = instance => Console.WriteLine("Persisting object " + instance);
    }

    public static void SetPersistenceDelegate(Action<object> persistenceDelegate)
    {
        _persistenceDelegate = persistenceDelegate;
    }
    public static Action<object> GetPersistenceDelegate()
    {
        return _persistenceDelegate;
    }

    public static void Persist(object instance)
    {
        _persistenceDelegate(instance);
    }
}

As you can see, our static helper now can swap the logic used in the Persist method by way of the SetPersistenceDelegate method, the GetPersistenceDelegate is there only to satisfy the unit test needs, we would like to reset the value of the delegate on each test (don’t worry, I’ll show this later). Another thing to note is that those two methods are public, well, I suppose we could make them internal and allow only the unit test assembly to use them, it is up to the reader to do that too. For the sake of this demo, I think it is ok (hint:InternalsVisibleToAttribute).

Now, let’s see how our unit tests look like:

public class Scenario3Tests
{
    #region Init, Before and After each test
    private Action<object> _originalDelegate;

    [TestFixtureSetUp]
    protected  void Init()
    {
        _originalDelegate = DataBaseHelper.GetPersistenceDelegate();
    }

    [SetUp]
    protected void BeforeEachTest()
    {
        DataBaseHelper.SetPersistenceDelegate(_originalDelegate);
    }

    [TearDown]
    protected void AfterEachTest()
    {
        DataBaseHelper.SetPersistenceDelegate(_originalDelegate);
    }

    #endregion

    [Test]
    public void when_disabling_a_product_its_enabled_property_is_set_to_false()
    {
        // Arrange
        var product = new Product
                          {
                              ProductId = 1,
                              ProductName = "Car",
                              Enabled = true
                          };
        var service = new ProductService();

        // Act
        service.DisableProduct(product);

        DataBaseHelper.SetPersistenceDelegate(instance =>
                                                  {
                                                      // no-op!
                                                  });
        // Assert
        Assert.AreEqual(false, product.Enabled);
    }

    [Test]
    public void when_disabling_a_product_it_persist_the_changes_using_the_product_repository()
    {
        // Arrange
        var product = new Product
        {
            ProductId = 1,
            ProductName = "Car",
            Enabled = true
        };
        Product productPassedToPersistMethod = null;
        var valueOfProductEnabledPropertyPassedToPersistMethod = false;
        DataBaseHelper.SetPersistenceDelegate(
            instance =>
                {
                    productPassedToPersistMethod = (Product) instance;
                    valueOfProductEnabledPropertyPassedToPersistMethod =
                                                 productPassedToPersistMethod.Enabled;
                });

        var service = new ProductService();

        // Act
        service.DisableProduct(product);

        // Assert
        Assert.IsNotNull(productPassedToPersistMethod);
        Assert.AreSame(product, productPassedToPersistMethod);
        Assert.AreEqual(false, valueOfProductEnabledPropertyPassedToPersistMethod);
    }
}

As you can see, the underlying idea of the tests remain the same, instead of using mocked repositories, we are using custom delegates and updating the helper delegate per test run to fit the test needs.

Finale

Well, I hope this article gave you an idea of how to unit test legacy code. Please bear with me if I’ve made some silly statement, if so, please let me know so I can learn a few tricks too. ;)

Also, I’ve not quite explained very well how a DI/IoC tool should be used to help us to resolve the dependencies at runtime. I’ll try to write another article to explain this. If you can’t wait, there are plenty of articles out there on the internet from where you can learn of this subject.

You can download the sample code from here.