Ending a Legacy

"Code without tests is bad code. It doesn't matter how well written it is; it doesn't matter how pretty or object-oriented or well-encapsulated it is is. With tests, we can change the behavior of our code quickly and verifiably. Without them, we really don't know if our code is getting better or worse."
Michael C. Feathers

Traditionally the term legacy code has been used to label software solutions that have been around for a while and has been developed with different tools from what we're currently using. Lately more and more people have started to label code without tests as legacy code. Tests help us improve quality, understand the software and reduce project risk, not to mention they make it possible to maintain a solution. I recently had to take on the responsibility for maintaining a set of solutions and frameworks developed by other developers. There was a catch though, all of the solutions where legacy solutions. The only responsible way of approaching this was to end the legacy. In this post I will lead you through the process of bringing a fictional* application under test. The application depends heavily on legacy frameworks, making the transition from legacy to maintainable even more challenging.

The process uses many of the techniques described in Michael C. Feathers excellent Working Effectively With Legacy Code which is a must-read whether you have to maintain legacy code or not.

* The scenario is based on a real-life application.

The Application

The application we're going to work with is an insurance contract maintenance service. The service depends heavily on a service delivery framework which dictates the design of the solution. The actual business logic, which we'd like to be testable, resides in a business service class called ContractBS. We'll get to know the other classes during our journey. We'll start by writing our first test. Bon voyage!

Running The Service From a Test

The first and foremost, we'll need to get acquainted with our test subject. The class is called ContractBS and encapsulates the business logic for retrieving insurance contracts for one or more clients. The class extends a super class named BusinessService which provides the base functionality for the service and mandates which operations need to be implemented by the service. The ContractBS service's super class implements the java.lang.Runnable interface and appears to be a worker object that depends on the services of a contextual object and cannot be run by it self.
A quick inspection of the facade class from which it is used reveals that the facade inherits a method called startService from its super class. This method accepts the business service instance, a generic DTO and the expected return type to be passed as arguments.

public abstract class WebService implements java.io.Serializable {
    // Additional methods and fields eluded.
    protected void startService(
        BusinessService service, 
        InputDO input, 
        Class returnClass) throws WebServiceFault
    {
        /* Compiled code */
    }
}

Unfortunately,  the startService method is protected so we cannot create an instance of the facade class deriving from the WebService class and use it to run our worker object. We could replicate the behavior of the WebService class in our test method, but this would lead to production code in the test fixture which is a particularly nasty test smell.
To be able to use the WebService class to test our business service, we need to create a test double for the facade service which exposes the startService method publicly so that we can pass it the business service we want to exercise.

public class FakeWebService extends WebService
{
    public void startService(
        BusinessService businessService, 
        InputDO inputDO, 
        Class clazz) throws WebServiceFault
    {
           super.startService(businessService,inputDO,clazz);
    }
}

By delegating the execution of the business service to our FakeWebService class we should be able to invoke the service from within a test method. Let's take a shot at it.

public void testCanRunSerivce() throws WebServiceFault
{
    FakeWebService serviceHost=new FakeWebService();
    ContractBS businessService=new ContractBS(null,null);
    serviceHost.startService(businessService,null,ContractDO.class);
}

Passing null to all of the constructors and methods is a neat trick to avoid creating objects we don't really need for our test. If we were to pass all of the required types to the constructors and methods above we would need to figure out what all five arguments are, instead of doing this up front we rely on the exceptions that are (hopefully) thrown by the startService method to figure out which arguments we need to supply.

When running our test we get a ExceptionInInitializerError, but the output on the console is what grabs our attention first.

0 [main] FATAL framework - A propertyException occurred:
(PR-1, Property file: "Log" does not exist (at least not in the classpath).]

Apparently, there is a missing property file (similar to configuration files in .NET). This issues could be easy to resolve by supplying one. But, since we have no idea of which properties to define and our goal is to test the business service rather than the framework it depends on, we'll try to find a way to remove the dependency on this property file.
Whenever we get an ExceptionInInitializerError it is because an exception has occurred during the initialization of a static variable or in a static initializer. 
The FakeWebService test double extends the abstract WebService framework class. We have the source code for this class, so we can find out what goes on during initialization. The WebService class has a static initializer (similar to a static constructor in .NET).

public abstract class WebService implements java.io.Serializable {
    // Additional methods and fields eluded.     
    static
    {
        Log4JStageConfigurator.configure("Log");
        GWRetry gw = new GWRetry();
    }
}

The error printed to the console is likely to stem from the call to the Log4JStageConfigurator class. Static initializers cannot complete abruptly by throwing exceptions, and the compiler is responsible for enforcing this. However NullPointerException is an unchecked exception which makes the compiler unable to enforce this rule if one of the invoked methods throws the exception. This explains why were getting the ExceptionInInitializerError. However we cannot know which of the two lines causes the exception.
Our next step will be to refactor the static initializer into a regular method.

When we make changes to legacy code, we should be really careful not to change the behavior of the code. This implies that we can only make small changes to the code before we get our tests in place. With the WebService class, we introduce a new Configure instance method, a static flag we can use to determine whether the configuration is done and we make a call to the Configure method from the constructor.

public abstract class WebService implements java.io.Serializable {
    static boolean isConfigured=false;
    protected void Configure()
    {
        if (!isConfigured) {
            Log4JStageConfigurator.configure("Log");
            GWRetry gw = new GWRetry();
            isConfigured=true;
        }
    }
    public WebService() {
         super();
         Configure();
    }
}

Notice that we are violating a good design practice with our change. We should not invoke virtual methods from within a constructor, because such calls will never go to a more derived class than that of the currently executing. However in this case we're doing this deliberately to be able to override the Configuration method in our test double. In essence, we replace a design that sucks, with one that sucks less. 

public class FakeWebService extends WebService
{
    protected void Configure()
    {
        // Do not perform configuration when testing...    
    }
}

When we have our tests in place we can refactor this code to a better design, now we just want to get the tests in place.

Let's Take Another Shot At Running The Test
Our second attempt to run the test gets us a past the previous exception, but this time were getting a NullPointerException from the startService method. Luckily, we get some new information from the console log.

Unable to parse SOAP-Header, so that we can populate InputDO.

This isn't a particularly good explanation of what is wrong, but it does give us some clues. The framework tries to construct an InputDO, but it unable to do so. Recall that InputDO was one of the arguments we passed null for, so let's try to provide a vanilla InputDO instance instead.

public void testCanRunService() throws WebServiceFault
{
    FakeWebService serviceHost=new FakeWebService();
    ContractBS businessService=new ContractBS (null,null);
    serviceHost.startService(businessService,new InputDO(),ContractDO.class);
}

When we run the test again it still fails, but we now get the WebServiceFault exception we're expecting. This exception contains some useful information:

[Error-1002 - Input to the service is incorrect. (app:null ref:0000--1185443152625) - (tms:1185443152640)]
[VALIDATE-1050 - UserId missing. - (tms:1185443152640)]

UserId is one of the properties on the InputDO, so we'll provide a dummy value for this property. After a few more failing testruns, we learn which properties we need to provide. We can then extract the creation of a minimal InputDO to a method on the test case and invoke this method from our test.

private static InputDO getMinimalInputDO()
{
    InputDO inputDO=new InputDO();
    inputDO.setUserId("userId");
    inputDO.setUserRole(InputDO.ADMIN);
    inputDO.setClientId("clientId");
    inputDO.setClientEnvironment("test");
    return inputDO;
}

public void testCanRunService() throws WebServiceFault 
{ 
    FakeWebService serviceHost=new FakeWebService(); 
    ContractBS businessService=new ContractBS (null,null); 
    serviceHost.startService(businessService,getMinimalInputDO(),ContractDO.class); 
}

When we run the test again, we get yet another WebServiceFault, but this time it is for different reason.

[Error-1002 - Input to the service is incorrect. (app:null ref:0000-1185443645046) - (tms:1185443645062)]
[Validate-1050 - personal identification number missing. - (tms:1185443645062)]

The personal identification number is a string argument on the ContractBS constructor, and when we replace the null with a dummy number we get a different exception.

[Error-1006 - The service failed (app:null ref:0000-1185444142828) - (tms:1185444142890)]
[Insurance-3000 - Some error in the insurance services - (tms:1185444142890)]
[DB-1500 - Connection to Database server failed (property file: ContractDB) -  (tms:1185444142890)]
[JDBC-1300 - Could not create JDBCSource (type generic) - (tms:1185444142890)]
[THROW-1 - java.lang.ClassNotFoundException: net.sourceforge.jtds.jdbc.Driver - (tms:1185444142890)]

Under normal circumstances getting exceptions is bad, but here it is an indication that we're making progress. However we don't really want to connect to the database when we test our service, so our next step is to get rid of the dependency on the database.

Mocking The Data Access Layer
By inspecting the source code for the contract business service we learn that it implements an abstract method from the BusinessService super class.

protected Data createObject() throws GeneralException {
    String sql =
        "SELECT * from CONTRACT_VIEW where pidnum = '" +
        idNumber + 
        "' AND (dat = (SELECT MAX(dat) FROM CONTRACT_VIEW WHERE (dat <= '" +
        ContractUtil.period2Date(period) +
        "'))) ORDER BY contractNumber";
    List contracts = InsuranceAccess.getContracts(sql);
    if (contracts != null && contracts.size() > 0)
         return (ContractDO) contracts.get(0);
    else
         return null;
}

This method builds a SQL query and passes it as an argument to a static helper method which on the data access class. We have a few options on how we can remove the dependency on the database. We can subclass the ContractBS class and override the createObject method with our own implementation. Below is an example of this.

public class TestingContractBS extends ContractBS
{
    public TestingContractBS (String idNumber, PeriodInputDO period)
    {
        super(idNumber, period);
    }

    protected Data createObject() throws GeneralException
    {
        // We would need to return the expected instance here.
        return new ContractDO();
    }
}

Another option is to replace the data access layer with a mock implementation. The subclass and override method approach is the simplest of the two, however in our application there are numerous methods that depend on the database so we're better off by mocking the entire data access layer.

The first thing we do is to rename the InsuranceAccess class to InsuranceAccessImpl, then we refactor all of the methods in the InsuranceAccessImpl class from static to instance methods and perform an interface extraction refactoring on the class. We then replace all the references to InsuranceAccessImpl with the InsuranceAccess interface. The last thing we need to do is to replace the constructor of InsuranceAccessImpl with a factory method.
The static method call in the createObject method now looks like this:

InsuranceAccess dao=InsuranceAccessImpl.createInstance();
List contracts=dao.getContracts(sql);

We've now refactored from a hard reference to a service registry pattern. To avoid making too many changes before we've gotten our tests in place, we'll leave the service registry in the InsuranceAccessImpl class.
To enable the test to provide their own implementation of the InsuranceAccess interface we need to alter the factory method to make the instance overrideable.

public class InsuranceAccessImpl implements InsuranceAccess
{
    public static InsuranceAccessdaoImpl;
    public static InsuranceAccesscreateInstance()
    {
        if (daoImpl==null)
        {
            return new InsuranceAccessImpl ();
        }
        else
        {
            return daoImpl;
        }
    }
}

Our next step is to write our own data access layer for use in the tests.

public class InMemoryDbDao implements InsuranceAccess
{
    private static final Pattern oidPattern = Pattern.compile(".*'([0-9]{9})'");
    private static final Pattern pidPattern = Pattern.compile(".*'([0-9]{11})'");
    private static final Pattern fromDatePattern = Pattern.compile(".*'([0-9]{8})'");
    private static Hashtable db = new Hashtable();
    private static int identity = 0;
    private String lastBid;
    private String lastPid;
    private Calendar lastDate;
    private EntrySelector selector;
    public class NullSelector implements EntrySelector
    {

        public boolean select(Object obj, InMemoryDbDao instance)
        {
            return false;
        }
    }

    public InMemoryDbDao ()
    {
        this.selector=new NullSelector();
    }
    public InMemoryDbDao (EntrySelector selector)
    {
        this.selector = selector;
    }
    public String getLastBid()
    {
        return lastBid;
    }
    public String getLastPid()
    {
        return lastPid;
    }
    public Calendar getLastDate()
    {
        return lastDate;
    }
    public static void clearDb()
    {
        db.clear();
    }
    public static ContractDO generateRecord(String pid)
    {
        ContractDO contract = generateContractDO();
        contract.setPid(pid);
        db.put(pid, contract );
        return contract;
    }
    public static ContractDO generateContractDO()
    {
        // Construct vanilla ContractDO
    }
    public List getContracts(String sql) throws InsuranceException
    {
        lastBid = null;
        Matcher oidMatcher = oidPattern.matcher(sql);
        while (oidMatcher.find())
        {
            lastBid =oidMatcher.group(1);
        }
        lastPid = null;
        Matcher pidMatcher = pidPattern.matcher(sql);
        while (pidMatcher.find())
        {
            lastPid =pidMatcher.group(1);
        }

        lastDate = null;
        Matcher fromDateMatcher = fromDatePattern.matcher(sql);
        while (fromDateMatcher.find())
        {
            try
            {
                DateFormat df = new SimpleDateFormat("yyyyMMdd");
                Date d = df.parse(fromDateMatcher.group(1));
                lastDate =Calendar.getInstance();
                lastDate.setTime(d);
            }
            catch (ParseException e)
            {
                lastDate=null;
            }
        }

        List list = new ArrayList();
        Iterator itr = db.values().iterator();
        while (itr.hasNext())
        {
            Object current=itr.next();
            if (selector.select(current,this))
            {
                list.add(current);
            }
        }
        return list;

    }
}

Our mock data access layer lets us populate it with pre-baked entities so that we can be confident about what is returned from the "database". It also extracts the parameters from the SQL query allowing us to validate that the expected values are used in the query. Finally it allows us to provide test specific entity selectors to control which entities are returned for a query.

Let's Make Some Assertions
So far we've only made the service run within our test without testing anything. Our next step is to perform some assertions on the data returned by the service. Let's complete our test.

public class ContractServiceTestCase extends TestCase
{
    protected void setUp() throws Exception
    {
        super.setUp();
        InsuranceAccessImpl.daoImpl=new InMemoryDbDao(new ContractSelector());
    }
    public void testCanRunService() throws WebServiceFault 
    { 
        FakeWebService serviceHost=new FakeWebService(); 
        ContractDO expectedResult= InMemoryDbDao.generateRecord("31127522222"); 
        ContractBS businessService=new ContractBS("31127522222",null); 
        serviceHost.startService(businessService,getMinimalInputDO(),ContractDO.class); 
        ContractDO actualResult = (ContractDO) businessService.getData(); 
        Assert.assertNotNull("The service should not return null.",actualResult); 
        Assert.assertEquals(expectedResult,actualResult); 
    }
  
}

When we run the test we're all green and our legacy application has started its journey back to being maintainable.

It requires quite a lot of effort to make a legacy application testable, and you need to apply different techniques from what you'd do if you we're TDDing the application. You'll need to violate quite a few best practices along the way to get make things testable. A rule of thumb is to try to isolate the different components of the solution your testing. The best way of achieving this is to break dependencies in the software you're retrofitting the tests for. There are numerous techniques for doing this, and I've only shown a few in this post.
Even if you'll not be able to chose the best designs to do this, you will do the groundwork needed to improve the design of the legacy application through the process of making it testable.
Improving the design of inherently bad legacy code is tedious, but the small and continuous design improvements are great motivators when you're deep down in source code that has been untouched since it was first written.