Back with the refactoring project

I spent a little time with the guys on the refactoring project last week. Of course, as is the way, pressure from the business for more functionality has reduced the amount of clean up work that they’ve been able to do. The good news is that the builds are still repeatable and most of the tests pass. The bad news is only most of the tests pass.

The first thing I did after checking out the latest source from CVS was build all the test harnesses. I was pleasantly surprised that most of them built and ran without error. I was especially pleased (and relieved) that all the FX tests still worked. One new test harness had been written and a few tests. The tests that failed were failing due to a recent code shuffle. Unfortunately the shuffle was done for what at first appeared to be good reasons but later turned out to be merely a case of an inexperienced developer wanting to brush some complexity under the carpet.

The failing test failed to build because the interface to the object had been changed and the test hadn’t been updated to accommodate the change. The interface change removed an interface that was being explicitly passed in to the constructor and then passed to a child object and instead had the child object run off and grab the interface from a process-wide static singleton object that was rapidly accumulating complicated and unrelated functionality.

The programmer responsible explained how this made things “better” because the relationships between the objects were less complicated. I pointed out that the complexity and coupling was still there it’s just that now it was less obvious. The implicit coupling via the singleton meant that the object was now impossible to test… I explained how the explicit parameterization allowed us to test the object using a mock service provider and that now we’d have to delete this test because it wasn’t possible to replace the singleton. At that the programmer agreed that the old way was better, he liked tests, and he just wished he had enough time to write some…

Whilst I was with them the project manager mentioned that there was a rather urgent new requirement for the FX code. It was yet another “special case”; one that had been missed when we’d done our initial work. We added the feature, test first, in a couple of hours; which was pretty cool…

First we adjusted the mocks so that we could set up the environment that was required. It was a little unusual; for some currencies, some traders only contributed the over night rate and didn’t bother with the tomorrow next rate. The software was correctly dealing with this situation and interpolating the tomorrow next rate but it was then adjusting the overnight rate and it shouldn’t have been. We adjusted the mock live data sources so that we could set up a currency with no TN point. We ran the test and got the expected, wrong, results. We then debugged into the code and worked out why it was wrong and fixed it; when displaying the ON spread, don’t subtract TN from ON if TN was interpolated… Ran the test again and we got the results that the traders required.

The interesting thing, for me was how the tests acted as compiled documentation. It’s been 6 months since I’ve looked at the code and yet it was easy to work out how to make the changes required because the tests showed us how the objects worked and the ‘documentation’ that they provided was provably correct - the tests ran and passed. Working out where to make the change was easy because the first step was obvious; take the test for ON and make a test for ON without TN… Once that was done we could stick a few break points in and step through the calculations as the mocked up live data feed changed. I’m glad nobody had added any cancerous singletons into this particular area of the code.