Using tests to help refactor a legacy codebase

I have been tidying up a legacy code base recently. Its actually my own code, and there’s plenty to be done. Of course it was written under hard time pressure and I was working on my own so the quality has suffered. My discipline was not at the level at which I would apply to a commercial project. Which is an interesting learning in itself.

Here are some (quite lengthy) notes about the process I went through to clean up this code.

The first thing I noticed about the codebase was that although I had a build file which did lots of cool things, it didn’t run the tests automatically.

Step One – re-factor the build script

I wanted to tidy up the build script. It was quite long and hard to understand. Also I want to distribute this project on Google code to people and so I wanted to make it possible for people to simply checkout the code, and type “go” to get a distributable package built.

One of the issues with this is the dependant libraries. So I introduced Ivy to manage this for me (other posts to come on this topic).

Secondly I wanted to compile and run source code, unit tests functional tests (i.e. tests which may use external resources and involve a set of objects working together) and hopefully acceptance tests.

In ant, doing all this can require quite a bit of duplication. So I broke out some macrodefs to handle compilation and execution (again I’ll post this as a separate item hopefully).

Ok so the build looks sweet. When I finally run it and get it to open the junit reports in a browser for me, I see heaps of errors and failures! oh dear. I obviously hadn’t been running the tests every time I checked in. Bad developer!

So, step one with the legacy code base is to get all the tests runnable from the command line ON THE DEV BOX (I have seen a project where this was not simple to do and so the devs relied on the “CI” build to do this for them. not fast enough feedback).

I would like to get a CI build set up for it, but not sure where I’m going to host it. I think that the precedence should be FIRST dev build, THEN CI. If you cant get feedback on your dev box you are asking for integration problems.

Step Two – Make the tests green

Most tests were easy to fix. There were several types of failure / error:

1) Error because of missing resources. Simple to fix, just needed to copy them onto the classpath.

2) Changed functionality where test not updated. Often I had changed the underlying code (e.g. formatting a value) and not updated the tests. This is mostly caused by not running ALL the tests every time a change is made (see kent becks’ JUnit Max

3) Incomprehensible failure – Ah. One of the tests had such a complex set-up that I just couldn’t remember what the hell it was supposed to be testing. Well, I could see what it was trying to test, but the scenario was so complex that it was hard to work out why it was failing.

Solution ? Rip it out. This might seem a little drastic. Infact in one case I simplified it greatly but in a couple of other places I simply removed the test.

My rationale for this is that a failing test that I cannot fix is providing NO VALUE. Infact it is probably providing NEGATIVE value and actually creating NOISE.

Besides, its always in version control if I REALLY want to get it back later.

Step Three – Remove compilation warnings

My project was written in Java 1.4 I was using “enum” all over the shop as a variable name, which is obviously not cool in 1.5. So I removed all the warnings. Again, Warnings are NOISE, adding accidental complexity to my codebase. A health warning here. I once worked on a codebase where development was done on a branch. Going round removing all the warnings everywhere on a branch can seriously affect your merge because there are heaps of small changes which can lead to conflicts. Although relatively safe in terms of refactoring the cost in terms of merge hell is probably not worth it.

I did not remove deprecations yet. Thats another step.

Step Four – Refactor the tests

This is the interesting one and the one that prompted this post.

When running the unit tests, I saw that a few of them were taking quite a long time. These tended to be of the kind which were testing random number generators. To do this, they were generating say 100,000 random numbers in a Gaussian distribution and then analysing the standard deviation of the results.

Also I noticed that some of the tests had many test methods all testing different functionality (for example I had a “Math” class which does heaps of things).

Firstly I created a new source folder for “Functional Tests” to put these longer running, more high level tests in. As I was adding the new Test Cases, I realised that what I was doing was re-organising the functional areas of the tests. For example I extracted a class called “GaussianRandomGeneratorTest” which used to be a single method on my uber-test case.

I realised that what I was doing was Refactoring the Test Cases to represent a new, structure, without refactoring any of the source. Interesting. This means that what I am doing is 100% safe, there is NO WAY I can introduce a bug into the code because I am not mutating it. And yet I am building up a new understanding and description of the code which can point the way towards how the code might look one day.

This is potentially a powerful technique. I realise that I have used the same technique on a previous legacy source base, without really conceptualising it. We extracted all the tests that worked for the area we wanted to change, and then re-structured some of them to make more sense.

In summary, refactor the tests! Its 100% safe and can give you the freedom to experiment with levels of abstraction and cohesion without risking any changes to the code.

Step Five – Remove unused code

This one is quite hard. But luckily in my case I had used deprecation to mark classes I no longer needed. So this stage will involve removing all deprecated classes. Note that I have the luxury to do this as no-one depends on my project. At a minimum I should make sure that nothing inside the codebase uses these deprecated classes.

Share

Why simple code is more cost effective

A while ago, I was performing a refactor in our codebase, pulling some functionality into a core domain object and then factoring it into some smaller, more focused objects.

My main drivers for making this change were:

1) Law of demeter

2) Hard to see all the paths of execution through the method

3) Nested if { ... } statements

Thinking about 2), a method which has an execution path which is difficult to understand immediately and intuitively (for normal beings that is) is likely to hide a bug. Infact this particular piece of code was doing just that which is why I was changing it.

So more complex logic leads to buggy code which leads to a higher cost of maintennance. The latter has a direct impact on the bottom line of your project, not to mention potential success of the software in its usability or even core function.

The refactor followed three steps.

1) Move the method into the Target class. This involved extracting a parameter which was really the knowledge of the client of Traget. I was a little concerned about Target knowing too much which leads to 2). I did this first by moving the method and then running all tests for client class, then I duplicated the test methods on the Target class and ran them (required a small refactor which made them easier to read as a side effect)

2) As i was doing this move, i realised that a scenario had not been tested, a consequence of 2) above, i.e. the path of execution was complex and so a test had been missed (if TDD had been followed this would be unlikely)

3) Finally, I wanted to get rid of the if statements. In order to do this, I used Replace method with method object to pull out the logic inside the if blocks.

Summary :

By decomposing the flow of logic into smaller more focused units, it makes the individual parts easier to understand and so it is more obvious if an error in the flow exists. It also helps to write test cases in a more modular way. Rather than 20 test methods in a test case you break into several test cases covering specific functionality.

One question arises, is refactor 3) a refactor too far ?

How do I decide this ?

The best way is to get someone else to look at the code and see if it reads well to them. If i was writing a book, I would get someone to proof read it so that I could confirm that my language was expressed in a clear way. This is the kind of review which Pair programming provides as standard, but even then it might be useful to show it to people outside the pair as you can both become entangled.

Share

When to stop refactoring (Stop, Look and Listen before you cross the road)

I have been working with Clive on a fair amount of refactoring the last couple of weeks.

The other day we encountered a classic case of refactoring choice and narrowly averted a potentially lengthy (and confusing to future developers) refactor. This was due to my pairs’ breaking the spell of the refactor path we were following.

Our problem was a circular dependancy. We were introducing a new service to the system to replace a heap of code sprinkled throughout the codebase with a central point of access. We had implemented our shiny new service and were moving around the codebase getting things to use it instead of doing all the work for themselves.

We reached a class and attempted to add a reference to our new module (we are working in C#). Doh! visual studio complains at us “you cant add that reference because you already depend on this module”. Apparently there are ways to circumvent this but I am quite a stickler for dependency management (think it was hammered in to me learning C++), and anyway who wants to mess with visual studio when its angry?

Solution 1

Ok, so lets move our dependancy into a module which is higher up the dependancy tree, problem solved. Well, yes, except that once we moved one, we discover a couple more. Plus we are working on a branch and in a codebase without too much of a safety net (ok lets not get into why we would be doing that), so we dont want to be changing the world.

Our Solution was move the class up into a new module, but leave the old one there, just delegating to the new implementation. Not ideal, but at least it leaves the code in an unambigous state (i.e. there are not 2 classes doing the same thing, infact with the same code.

We had reached a point where we were beginning to feel uncomfotable with this. It was feeling heavyweight and intrusive, and worse, felt like we were creating technical debt. this situation would really need to be tidied up later.

We broke for lunch, in the hope that we may have some inspiration with food. After lunch, I returned to find Clive browsing the codebase:

“Hey Clive, did you get any genius ideas over lunch ?”

“Well, I was just looking through the code, and i was wondering, what was it we were trying to achieve again ?”

It was like smelling salts:

“Of course, that’s great problem solving skills, we’ve been going about this all the wrong way!”

The answer was obvious. Instead of moving half the code all over the place because of this one problem child, we had to change that child to be less greedy for responsibility. It was doing too much.

All we had to do was stop it getting its own resources, and instead, pass them in, injecting through the constructor. This pushed the responsibility for obtaining the resource to the client.

As it happened, the client was also in the common module, but was a couple of static methods. A simple move method refactor moved these methods down into a module which we didnt depend upon and hey presto our dependancy circle was broken.

I think this is a great example of how you can get sucked in to a solution. To begin with we found it hard to see another way out of the problem. Simply stopping, looking at the problem form the beginning again, and listening to the code which was telling us not to cross the road, we were able to walk up the street for a few metres and find a safe zebra crossing.

Share

What does “Refactor” mean ?

I was reading Kent Becks’ Smalltalk Patterns1 and he mentioned quite early on that it is important to factor the design correctly in terms of small, cohesive classes. It wasn’t until I read this that I had made the connection between “Refactor” and “Factor” and hence the mathematical definition of a factor.

Suddenly I thought I saw what he was talking about. Code can be broken down into factors which is when it becomes more flexible or supple (in the language of domain driven design).

In mathematical terms, a factor is a number which can be combined with another number to produce a given result. So the factors of 10 are {1, 2, 5 and 10}. Those of 12 are {1, 2, 3, 4, 6 and 12}.

Let’s say you represent a piece of code as say 12, perhaps it is one big class doing several things. Ok, you can have one big class, maybe its easy to find everything there because its all in one place. However, you could also break it into two classes. Perhaps one is equivalent to say 3 and the other is 4. So 3 * 4 still equals 12 right?

What happens if you factor the 4 into {2, 2}. Now you have {2, 2, 3, 4} You can now make quite a few different “products” of these factors, not just 12, e.g. 2 * 3 = 6. Further, you can make combinations of these intermediate products aswell, e.g. 2 * 2 * 4 = 16

I thought this was an interesting way to look at factoring (or re-factoring), and possibly a useful analogy to explain it. I had not heard it being talked about in this way before.

1 Kindly lent to me by Pat

Share

Encapsulation, What is it Good for ?

I just had a moment of clarity about refactoring and encapsulation. Its a simple truth and is one of the basic tenants of the concept and I had kind of forgotten about it.

It occured to me because I was working with some javascript. Its very easy to end up with lots of global methods in JS unless you use your closures properly.

By putting variables inside functions you can make them private.

Why is this useful. Well consider that you want to CHANGE one of those functions. Change ? who would EVER want to do that ?

If it is declared as a private function you KNOW FOR SURE that no-one else is using it and therefore that YOU CANNOT BREAK ANY OTHER CLASSES.

Therefore it is COMPLETELY SAFE TO REFACTOR.

great isnt it!

Such a thing you take for granted and yet when you work with a different language domain it suddenly reminds you of all the good things about OO.

Share