design, legacy code, unit testing

Refactoring legacy code – how to get them singletons under test

A couple of weeks ago I bought the book “Working Effectively with Legacy Code” (WELC) by Michael Feathers (MF). I’m now more than halfway through this excellent book and can’t help to regret I didn’t buy this one long time ago. It is an absolutely “must have” for all programmers working with legacy code and who are struggling to get these nasty systems under test.

In a recent post I discussed how I was able to build a service layer on top of a legacy code base and successfully unit test these services using the adapter pattern. In order to create the required services we needed to try and understand the legacy code we were interfacing with. With over 1000 classes, some of them over 5000 lines long and with 0 % unit test coverage, this was very challenging indeed. This code base is practically crying for some refactoring and with this great book in my possession, I wanted to try out some of the theories. Here’s how it went…..

First attempt of getting the legacy code under test

I found one of the biggest classes in the code base (I’ll call it Account in this post) and proceeded to write a test for one of the couple of 100 methods in the class. First step: Try to instantiate the class under test. Lets see, a couple of alternatives, lets go for the least complex one. Bad luck, in order to create an instance of this class a connection to the database is required. After some investigation it turns out that the constructor instantiates another object which again requires some configuration values stored in the database. More bad luck. The class loading the required configuration values is a singleton and what’s even worse is that it loads its variables in a static initializer. This basically means that we have no control over the construction of these database connections. A quick search through the code base reveals that there are hundreds of these singleton objects and usage of them is spread all over the place.

The problem

The code snippet below shows the problem. In the construction of the Account object we suddenly encounter the following code line:

public Account(int accountId){
   ...
   AccountConfigurationTable.getInstance().getAccountTypes()
   ...
}


MF has a nice section about how to get singletons into a test harness, in the section “The case of the irritating global dependency”. I think definitely some of the tips could come handy when trying to get this system under test.

The figure above shows a simplified class diagram of the configuration table “design”. The class we need to get under test is, as mentioned above, the Account class. This class has amongst others a global dependency to the AccountConfigurationTable class. From the diagram we can see that this class inherits from an abstract class AbstractConfigurationTable which again uses a concrete DataSource class. Finally, the DataSource class inherits from the SQLDataSource. One could argue that the inheritance hierarchy is too complex and it certainly makes our job of testing more difficult than it should be. In the following we’ll see how we can fix these problems

Step 1 – getting rid of the static initializer

Getting rid of the static initializers is the first step in order to get this code base under test. The little evil initializers makes it really hard for us to unit test classes in the code base. The reason is that we have no control over the construction process. The code below shows how this is done today.

static {
   try {
      INSTANCE = new AccountConfigurationTable();
   } catch (Exception e) { //Note: Exception handling simplified
      throw new SystemException(e);
   }
}

And the getInstance() method:

public static AccountConfigurationTable getInstance() {
   return INSTANCE;
}

Removing the initializer we need to do the same work in the getInstance() method constructor. This will preserve the behavior in the production code since users of the singleton needs to call this method in order to get the singleton instance. Our new code looks like this:

public static synchronized AccountConfigurationTable getInstance() {
   if (INSTANCE == null) {
      try {
         INSTANCE = new AccountConfigurationTable();
      } catch (SomeException e) { //Again: Exception handling simplified
         throw new SystemException(e);
      }
   }
   return INSTANCE;
}

Step 2 – Loading an alternative singleton instance during tests

But this is not quite enough. We also need to set an instance during tests and therefore decide to use the “Introduce static setter” technique (from WELC – “The case of the irritating global dependency”):

public static void setTestingInstance(AccountConfigurationTable testInstance)

By doing this we can inject a testing instance of this singleton when testing the Account class.

Second attempt of getting the legacy code under test

Ok, we are now ready to write a test. We set up the testing instances of the singleton in the setUp method, write the code for for the method under test.. red bar. Looking at the source code we discover that the SQLDataSource class of course connects to the database

Step 3 – Introduce an interface for the concrete DataSource

We need to replace the concrete data source with a test data source. Luckily we see that the AbstractConfigurationTable class instantiates a DatabaseDataSource class (name invented for this article solely, so don’t bother about the bad name). This means that we can inject another data source if we use an interface instead of a concrete class. We can do this by using the refactoring technique “Extract Interface” on the DatabaseDataSource class, let’s call it DataSource. In addition we must refactor the AbstractConfigurationTable class to refer to this interface instead.

Step 4 – Find a way of injecting the type of DataSource to use

The last thing to do is to inject the DataSource (dependency injection principle), in this case I favor constructor injection. Thus, we add a second constructor accepting a DataSource as argument:

public AccountConfigurationTable(DataSource testInstance) {
   super(testInstance); //Call the super class constructor explicitly in order to set DataSource
   this.dataSource = testInstance;
}

em>(Note: This constructor should only be used for testing purposes – at least until the loading of the configuration classes is refactored. Therefore it might be wise to enforce that no one calls this constructor from production code. This may be done for example using aspects that check violations on this rule)

The figure below shows the new improved class diagram (still simplified):

Step 5 – Replacing the configuration values retrieved from DB

This leaves just one problem. Our ConfigurationTable classes stores the database values in a HashMap. So far we have only replaced the database connectivity when doing tests but we have not introduced any alternatives that fills out this HashMap with values. There are of course several alternatives. for now we will settle with loading of a set of configuration values through a setter method. This way we can build up a HashMap and inject it into the ConfigurationTable class during tests.

Third attempt of getting the legacy code under test

We are now ready to run our unit test again…. Green bar. We have successfully replaced the global dependency to our two database sources when testing and are in a position to write some unit tests. There are of course plenty of challenges left to solve in order to get this code base under test but I truly believe that it’s necessary to start small and improve the system doing small changes. Eventually, we will have enough tests to start more heavy refactoring which again will lead to a better design for the system…

Summary

To summary, the steps we performed to get the Account class under test was:

  1. Replace static initalizers in DB classes with constructor initialization.
  2. Add a static setter method for injecting a test instance of the DB class (“Introduce static setter” in WELC)
  3. Use “Extract Interface” on the concrete DataSource class so that we can replace it with an alternative source in a test harness.
  4. Override the default constructor on the AccountConfigurationTable with one accepting the alternative data source and call the similar new constructor on the parent class AbstractConfigurationTable
  5. Make it possible to inject the configuration values into the AccountConfigurationTable class – for example using setter injection

References

1. Working Effectively with Legacy Code (2005) – Michael Feathers

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s