No, I’m not being ironic; I’m actually enjoying working with legacy code!
I have recently inherited a large, legacy code-base. There are hardly any tests and it’s full of dependencies between components. The application is critical to the day-to-day operation of the business and requires ongoing feature additions, support and maintenance.
By “legacy code”, I am referring to the definition as ”code without tests and in need of refactoring”. This is described by Michael Feathers in his excellent book Working Effectively with Legacy Code. This post uses techniques from this book. If you are working with a legacy code base, buy this book! It will make your life much easier.
As developers, most of us have had to endure the frustration of working with code that is unclear and untested (if you haven’t, you don’t know what you’re missing 
. It can be painful having to make changes without knowing the affect the change could have on the application. We usually apply Change-and-Pray Development, that often results in hard-to-find problems slipping through to production. Over time, constant tacking-on of changes further degrades the quality of the code-base and can result in a Big Ball of Mud.
But fear not, there is a way to make changes to legacy code much easier, and – dare I say, fun! Continuous refactoring and testing can help to turn a bug-ridden, untested and tangled application into a slick, streamlined code-base that just gets better over time.
Introducing Changes To Legacy Code
One of the most difficult things about working with legacy code is introducing changes. Without tests to back us up, it is hard to know if a change has worked and hasn’t broken anything unexpectedly.
For this demonstration, I have created a sample class that reflects some common issues when introducing changes to legacy code. The code is intentionally simple to clarify the refactoring. Real-world legacy code is not usually so simple, but these techniques still apply.
Here we have a DocumentManager class.
public class DocumentManager
{
public static Document GetDocument(int documentId)
{
DocumentDAL documentDAL = new DocumentDAL();
Document document = documentDAL.Get(documentId);
if (document != null && !File.Exists(document.Path))
{
throw new FileNotFoundException("Document file was not found");
}
return document;
}
public static List<Document> GetAllDocuments()
{
DocumentDAL documentDAL = new DocumentDAL();
List<Document> documents = documentDAL.List();
foreach (Document doc in documents)
{
if (doc != null && !File.Exists(doc.Path))
{
throw new FileNotFoundException("Document file was not found");
}
}
return documents;
}
public static void SaveDocument(Document document)
{
File.WriteAllText(document.Path, document.Contents);
DocumentDAL documentDAL = new DocumentDAL();
documentDAL.Save(document);
}
public static void DeleteDocument(int documentId)
{
Document document = GetDocument(documentId);
DocumentDAL documentDAL = new DocumentDAL();
documentDAL.Delete(document);
File.Delete(document.Path);
}
public static string GetFileName(Document document)
{
return Path.GetFileName(document.Path);
}
}
The purpose of this class is to manage the persistence of documents to a database and file system.
I have come across this type of class many times; it is mostly composed of static methods that call an underlying data access layer. The class is doesn’t have any unit tests and contains direct calls to the file system and a data access class.
My goal is to add some validation code into the Save method of the DocumentManager class and to create unit tests to verify the new functionality.
Firstly, I’m going to create a test harness so I know my changes won’t adversely affect the existing functionality.
Creating a Test Harness
Creating a test harness allows us to make changes while ensuring the code still functions as expected.
Let’s create a new test fixture called DocumentManagerTests.
[TestFixture]
public class DocumentManagerTests
{
}
Instantiating the Class
Now that we have a test fixture in place, let’s create a test that instantiates the class we want to test.
[Test]
public void Can_Create_Instance_Of_DocumentManager()
{
DocumentManager documentManager = new DocumentManager();
Assert.That(documentManager, Is.InstanceOfType(typeof(DocumentManager)), "Should create an instance of DocumentManager");
}
Our code doesn’t compile, as DocumentManager is a static class and can’t be instantiated. We could just test the members of the static class, but we need an instance so any dependencies can be passed into the constructor. We will come to this shortly, but for now, let’s remove the static keyword from the class declaration so we can instantiate the class.
public class DocumentManager
{ ...
After making the class non-static our test compiles and passes! The assertion isn’t very meaningful, but at least we know we can instantiate the class without an error.
Breaking Dependencies
The reason legacy code seems difficult to test, is that it often contains internal calls to other components or sub-systems. This makes it difficult to isolate a piece of code for testing.
So why break dependencies and not just test everything? Well, firstly you end up testing the integration between components, rather than a single unit of code. Calling external components can cause our tests to run slowly, or to fail due to an problem with a sub-system.
To test the Save method in isolation, we need to extract calls to the DocumentDAL and File classes.
These are fairly safe structural refactorings, so we should be able to make the changes without affecting functionality.
To remove the dependency on the DocumentDAL class in the Save method, we can use a technique called Dependency Injection.
Dependency Injection
Dependency Injection involves passing a reference to the dependent object into the class constructor. Here, we can use an Extract Interface refactoring to decouple the DocumentDAL implementation from its interface. Using an interface allows us to provide a mock implementation for testing.
public class DocumentDAL : IDocumentDAL
{ ...
We can now “inject” the dependency into the DocumentManager constructor.
public class DocumentManager
{
private readonly IDocumentDAL documentDAL;
public DocumentManager(IDocumentDAL documentDAL)
{
this.documentDAL = documentDAL;
} ...
Our test doesn’t compile, as we need to pass an implementation of IDocumentDAL into the constructor. I am using Rhino Mocks to mock an IDocumentDAL implementation.
[Test]
public void Can_Create_Instance_Of_DocumentManager()
{
MockRepository mocks = new MockRepository();
IDocumentDAL documentDAL = mocks.CreateMock<IDocumentDAL>();
DocumentManager documentManager = new DocumentManager(documentDAL);
Assert.That(documentManager, Is.InstanceOfType(typeof(DocumentManager)), "Should create an instance of DocumentManager");
}
To remove the Save method’s dependency on the file system, we can use another dependency-breaking technique called Subclass and Override Method.
Subclass and Override Method
First, we use Extract Method to move call to the file system into a separate virtual method.
protected virtual void WriteFile(Document document)
{
File.WriteAllText(document.Path, document.Contents);
}
We need to change the Save method to call the new WriteFile method. Because WriteFile is an instance method, we need to make the Save method non-static. Note that any code that calls this method will also need to be changed to use the non-static method.
public void SaveDocument(Document document)
{
WriteFile(document);
DocumentDAL documentDAL = new DocumentDAL();
documentDAL.Save(document);
}
Now we can create a subclass of DocumentManager called TestingDocumentManager and override the WriteFile method. This allows us to stub-out the calls to the file system by the Save method for use in our tests.
public class TestingDocumentManager : DocumentManager
{
public TestingDocumentManager(IDocumentDAL documentDAL)
: base(documentDAL)
{
}
protected override void WriteFile(Document document)
{
}
}
Next, we update our test to use the TestDocumentManager with the overridden WriteFile method.
[Test]
public void Can_Create_Instance_Of_DocumentManager()
{
MockRepository mocks = new MockRepository();
IDocumentDAL documentDAL = mocks.CreateMock<IDocumentDAL>();
DocumentManager documentManager = new TestingDocumentManager(documentDAL);
Assert.That(documentManager, Is.InstanceOfType(typeof(DocumentManager)), "Should create an instance of DocumentManager");
}
Ideally, we could remove the dependency on the file system altogether by injecting an interfaced wrapper for the File class, but that would distract us from the focus of our test. Subclass and Override Method is a very useful technique for quickly getting legacy code under test. We can refactor the code at a later time if required.
Testing the Method
To test the Save method we need to remove the instantiation of DocumentDAL and use the field that was set in the constructor.
public void SaveDocument(Document document)
{
WriteFile(document);
documentDAL.Save(document);
}
Now we can test the functionality of the Save method.
[Test]
public void Can_Save_Document()
{
MockRepository mocks = new MockRepository();
IDocumentDAL documentDAL = mocks.CreateMock<IDocumentDAL>();
DocumentManager documentManager = new TestingDocumentManager(documentDAL);
Document document = TestHelper.GetDocument();
Expect.Call(() => documentDAL.Save(document));
mocks.ReplayAll();
documentManager.SaveDocument(document);
mocks.VerifyAll();
}
Running this test shows us that the behaviour of Save is working as expected. It provides a safety net for making further changes. If a change affects the core behaviour of the Save method, this test should fail.
Making the Changes
Now that we have a test harness in place, we can implement our new changes using Test Driven Development (TDD).
First, we write a test to verify the changes we want to make.
[Test]
public void Cannot_Save_Invalid_Document()
{
MockRepository mocks = new MockRepository();
IDocumentDAL documentDAL = mocks.CreateMock<IDocumentDAL>();
DocumentManager documentManager = new TestingDocumentManager(documentDAL);
Document document = TestHelper.GetInvalidDocument();
try
{
documentManager.SaveDocument(document);
Assert.Fail("Should throw an exception for invalid document");
}
catch(Exception ex)
{
Assert.That(ex, Is.InstanceOfType(typeof(ValidationException)), "Should throw a ValidationException");
}
}
The code compiles and the test is failing as expected. We now need to implement the code to make the test pass.
public void SaveDocument(Document document)
{
if(!document.IsValid)
{
throw new ValidationException();
}
WriteFile(document);
documentDAL.Save(document);
}
The test passes! We now run all the tests to ensure our change hasn’t broken the existing functionality.
Refactoring
With our tests passing, we can now clean things up a bit. We are free to make changes on any code that has been tested.
We could also take the time to further improve the design of the DocumentManager class, such as separating the File Access responsibilities into another class. The amount of refactoring you do depends on how much time you have. If you can spend some time tidying things up around the code you have changed, the better the code will be for the next person who needs to change it. You don’t have to do everything at once. Generally, code that is changed often should become continuously refactored and improved over time.
Summary
This is just one of many techniques you can use to refactor and test legacy code. Many more examples can be found in Working Effectively with Legacy Code. This has been a very simple example, but I have used these same techniques to make big changes to some very dodgy pieces of code.
Refactoring and testing legacy code can help to improve a legacy code-base over time. It feels like you’re writing new code, rather than just tacking onto existing code. It is also a great feeling walking away from implementing a change, knowing that the surrounding code is now cleaner and more robust than before.
