Mark Needham

I recently watched a presentation given by Joshua Kerievsky from the Lean Software & Systems conference titled ‘The Limited Red Society‘ in which describes an approach to refactoring where we try to minimise the amount of time that the code is in a ‘red’ state.

This means that the code should be compiling and the tests green for as much of this time as possible .

I think it’s very important to follow these principles in order to successfully refactor code on a project team and it’s an approach that my colleague Dave Cameron first introduced me to when we worked together last year.

These are some of my observations and thoughts on the talk:

• Kerievsky talks about parallel change which is where we want to make some changes to a bit of code and instead of changing it directly we create a parallel implementation and then gradually change the clients of that code to use the new approach.

  An example of this which I wrote about last year was when we wanted to move the creation of objects from using object initializer to the builder pattern. Instead of doing it all in one go we had both approaches in the code base at the same time and gradually moved all the existing code to use the new approach. We also tried to use this approach to change the API of one of the main objects in the CC.NET code in a dojo last year.

  Kent Beck talks about a similar approach in his QCon talk titled ‘Responsive Design‘ from November 2008.

• One interesting point that Josh made in the Q&A session is that there may be times when we don’t necessarily want to go straight for parallel change – it might be easier to use a narrowed change approach first.

  With narrowed change we would first look to reduce the number of places where we have to make the change we want to make.

  For example if we want to change an object to internally use a list instead of an array we could first isolate the places where we add to or retrieve from the data structure into methods. We would then call those methods instead of accessing the data structure directly.

  This way we can reduce the number of places we need to change when we eventually change the array to a list.

  I haven’t used this approach before but will look to do so in future.

• There was a discussion about small steps and big leaps at the end where one of the attendees pointed out that it often takes much longer to take a small steps approach rather than just taking one big leap.

  Josh pointed out that the appropriate choice depends on the risk involved in the refactoring situation – if it’s low risk then perhaps it does make sense to just change the code in one go. However, an additional advantage of the small steps approach is that it makes it much easier to do a graceful retreat if the refactoring gets out of hand and we end up yak shaving.

Although I already knew some of the approaches shown in this video it’s always interesting to see how experienced practitioners put them to use and there were a couple of new ideas that I hadn’t come across before.

I particularly liked the fact that there was a 20 minute Q&A section at the end. The discussion is quite interesting to listen to.

I’ve written previously about the Mikado method and how I’ve made use of it for identifying ways in which I could refactor code but I think this approach is more generally applicable for any kind of code investigation.

Our application has a lot of calculations in it and we’ve been trying to refactor the code which wires all the calculators up to make use of a DSL which reveals the intention of the code more as well as making it easier to test.

Unfortunately after changing the code to use this approach one of the calculations was about by about £15.00 in one of our acceptance tests.

We didn’t have the best logging around all the calculators so it wasn’t immediately obvious where to start looking for the problem.

I decided to sketch out the ways the calculators interacted with each other and then follow the deepest path – which was Calculator D – Calculator G – Calculator H – to see if there were any differences in the values being calculated when running the old and new versions of the code.

Interestingly there was no problem in that bit of the code which then allowed me to rule out that whole part of the tree and then start looking at the other calculators to try and find the problem.

I’d previously been trying to work out what was going on just by reading through the code but found it incredibly difficult to remember where I’d already investigated so drawing the diagram really helped with that too.

I wrote a few weeks ago about incremental refactoring using a static factory method where we ended up with the following code:

public class LookUpKey
{
	private readonly string param1;
	private readonly string param2;
	private readonly string param3;
 
	public LookUpKey(string param1, string param2, string param3)
	{
		this.param1 = param1;
		this.param2 = param2;
		this.param3 = param3;
	}
 
	public static LookUpKey CreateFrom(UserData userData)
	{
		var param1 = GetParam1From(userData);
		var param2 = GetParam2From(userData);
		var param3 = GetParam3From(userData);
 
		return new LookUpKey(param1, param2, param3);
	}
 
	public string Param1Key
	{
		{ get { return param1; } }
	}
 
	...
}

The next step after this refactoring that we wanted to drive was to push the logic out of the static factory method so that it could just be done when those properties were evaluated.

We wanted to drive the code into something similar to this:

public class LookUpKey
{
	private readonly UserData userData;
 
	public LookUpKey(UserData)
	{
		this.userData = userData;
	}
 
	public static LookUpKey CreateFrom(UserData userData)
	{
		return new LookUpKey(userData);
	}
 
	public string Param1Key
	{
		{ get { return GetParam1From(userData); } }
	}
 
	...
}

Unfortunately we also hydrate the ‘LookUpKey’ from the database when we’re loading ‘LookUps’ into memory so that we can query them in memory.

public class LookupRepository
{
	public Lookup Find(LookupKey lookupKey)
	{
	     var query = currentSession.Linq<LookupRecord>();
 
		// this result would be cached and then queried but for the sake of the example I don't show that
 
	     var lookupRecord = query.Where(l => l.LookupKey.Equals(lookupKey)).FirstOrDefault();
 
		if(lookup == null) throw Exception(...);
 
		return lookupRecord.Lookup;
	}
}

We are therefore mapping directly to those fields to hydrate the object. The Fluent NHibernate mapping code looks like this:

public class LookupRecordMapping : ClassMap<LookupRecord>
{
    public LookupRecordMapping()
    {
        ...
 
        Component(lookupRecord => lookupRecord.LookupKey, lookupKey =>
        {
            lookupKey.Map(x => x.Param1).Access.CamelCaseField();
            lookupKey.Map(x => x.Param2).Access.CamelCaseField();
            lookupKey.Map(x => x.Param3).Access.CamelCaseField();
        });
    }
}

The database table for that is as follows:

Param1 | Param2 | Param3 | LookupValue1 | LookupValue2

Looking at the code like this makes it more obvious that ‘Param1′, ‘Param2′ and ‘Param3′ together represent a concept and that ‘UserData’ probably doesn’t belong inside that object in the first place.

Rafael Peixoto de Azevedo wondered in the comments on the original post whether ‘Param1′, ‘Param2′ and ‘Param3′ represent a relevant concept for ‘UserData’ but interestingly in this case the business guys didn’t have a name for this concept so we decided that it was some sort of lookup.

I found it interesting that looking at this code from a different part of the system made it clearer that the refactoring I wanted to make didn’t actually make sense.