My own legacy code

I’ve just started work on an ISO8583 server for a client. I’ve done similar work for them in the past and the first thing that I did was to take the basic shell of the last server that I did for them and update it to use all of latest version of The Server Framework. The next was to start to look at the first of the requirements for the new server. It was then that I realised that I was working with legacy code…

This is the third ISO8583 server that I’ve done for this particular client. They’ve all shared a fairly similar shell and that dates back to early 2002: A windows service, with performance counters that uses our IO Completion Port Socket Server framework to provide a high performance server that receives ISO8583 messages, talks to a database and makes transaction authorisation decisions. The client has been very happy with the performance and reliability of the code, and so have I, but looking at it now, in 2005, lots of it looks like legacy code to me…

I guess I’m getting picky in my old age, but that’s hardly news. The code is legacy because, as Working Effectively with Legacy Code Michael Feathers says, it’s code without tests. It’s not that I developed the original servers without testing them, it’s just that the testing was done at a different level; I wrote our C# socket tester for the original project and we tested the server logic by having the C# client fire appropriate messages into the server and confirm that it got the expected responses. Now I know, from experience, that I’ll be able to finish this assignment much faster if I test at a lower level in addition to using the black box C# tester.

The first thing I wanted to do was to start to understand the spec by sketching out some code to get my head around the message processing required. I have a class called CMessageProcessor which is pretty much the heart of the business logic of the server. This takes complete, distinct, ISO8583 messages as input and does the business logic and returns an ISO8583 response message. Right now this part of the server will be my main focus as I’m interested in how the business logic of this new server will work. So my first job was to get CMessageProcessor under test and, as usual in legacy code, this was made more complex due to its reliance on concrete classes rather than interfaces.

I guess I should be thankful that I wrote the original code… I’d rather be working with my 3 year old code than certain other client codebases I could think of. Though CMessageProcessor was coupled to several other concrete classes at least the coupling was via objects that were passed in to CMessageProcessor. There was no insidious coupling via globals or singletons…

I set up the test harness and wrote the first test. As ever, the first test is the one that exposes all the explicit and implicit coupling that a class has. To be able to instantiate a class in the test harness requires that we can create every object that the object under test requires and that the test harness can link with every object that the object under test uses. The dependencies weren’t too bad, an error logging object and details of the database connection were passed in and we used the various ISO8583 message classes and created a database connection object from the connection details.

The constructor looked a bit like this:

CMessageProcessor::CMessageProcessor(
   const CErrorLog &errorLog,
   const CDatabaseConnection::ConnectionDetails &details)
   :  m_errorLog(errorLog),
      m_connectionDetails(details),
      m_pConnection(0)
{
}

The error log was easiest to deal with, the addition of an interface decoupled us from the concrete error log object and allowed us to slip in a mock which, for now, does nothing at all.

The database connection details class was just a bundle of connection details, provider, login details, database details, timeouts, etc. This class was then used later on to create a database connection. Rather than passing in a mock set of connection details it was better to pass in a ‘connection factory’. This not only removed our reliance on the connection details but also removed the creation and subsequent destruction of the database connection from within the object under test. The resulting constructor looked something like this:

CMessageProcessor::CMessageProcessor(
   const ILogErrorMessages &errorLog,
   const IDatabaseConnectionFactory &connectionFactory)
   :  m_errorLog(errorLog),
      m_connectionFactory(connectionFactory),
      m_pConnection(0)
{
}

The connection factory looked a bit like this:

class IDatabaseConnectionFactory
{
   public :
 
      virtual CDatabaseConnection *CreateConnection() const = 0;
 
      virtual void DestroyConnection(
         CDatabaseConnection *pConnection) const = 0;
 
   protected :
 
      ~IDatabaseConnectionFactory() {}
};

At this point we can easilly create mocks for the error log and connection factory and write test 1 to create the object under test in our test harness.

void CMessageProcessorTest::TestConstruct()
{
   CMockErrorMessageLog errorLog;
 
   CMockDatabaseConnectionFactory connectionFactory;
 
   CMessageProcessor processor(errorLog, connectionFactory);
 
   connectionFactory.CheckNoResults();
 
   errorLog.CheckNoResults();
}

Of course, test 1 doesn’t appear to do a great deal and at this stage. But it’s important because we have got the class under test. To be able to build and execute this test we’ve created or linked with all of the objects that this class requires.

At this point I like to chase these changes back through the code by building the rest of the application. I do this just so that I know I haven’t broken anything. When converting concrete classes to interfaces you can usually repeat the replacement all the way back to the point where the concrete class is actually constructed. In the case of the connection details change I managed to chase the change back all the way to the point where the connection details were created and at this point I stopped. I need a new class, a real connection factory and at the moment I think it’s slightly too early to create that class.

The reason I think it’s slightly early is that I can see another interface that needs to be introduced. At present we return a pointer to a concrete instance of our database connection from our connection factory. That will cause us problems later when we want to test the object under test’s interaction with the database without using the database… By changing the connection factory so that it returns a new interface we can completely remove our database from these tests. The factory becomes this:

class IDatabaseConnectionFactory
{
   public :
 
      virtual IDatabaseConnection *CreateConnection() const = 0;
 
      virtual void DestroyConnection(
         IDatabaseConnection *pConnection) const = 0;
 
   protected :
 
      ~IDatabaseConnectionFactory() {}
};

This change can then be chased through the object under test and another concrete class dependency can be removed. Almost…

A while back I mentioned that I think I went through a phase of overusing nested classes. Well, the database connection contains a nested class for its Exception. Rather than having CDatabaseConnectionException I have CDatabaseConnection::Exception. This isn’t a big problem at present, it means that we need to link with the database connection code, but even if I moved the exception class out so that it wasn’t nested it would still, logically, reside in the same library so the linkage issue doesn’t change that much. For now I’ll leave it as it is.

At this point I feel comfortable to create a test for the connection factory and create the new class so that the rest of the application builds again. The connection factory test doesn’t do a great deal, it just proves we can construct it, but the test is there and that will nag at me when I start to work on the object later…

Getting objects under test in legacy code isn’t always this easy, but the principles are the same; break dependencies and reduce coupling so that you can get a simple construction test to build and then mock up the objects that the object under test uses so that your tests can run quickly and without the need of other objects. Sometimes it seems like you’re making the legacy code more complex by adding in interfaces and breaking dependencies but the complexity was already there, it just wasn’t quite so obvious. As I’ve said before, I prefer complexity to be in your face rather than hidden.

Now seems about time to check in all my changes and when that’s done I can get back to the spec and start writing some tests for this new functionality, I hope…