Mark Needham

One of the requirements that the ThoughtWorks University grads have been given on the internal project they’re working on is to ensure that they leave the code base in a good state so that the next batch can potentially continue from where they left off.

The application will be deployed on Thursday and this means that a lot of the time this week will be spent refactoring certain areas of the code base rather than only adding new functionality.

When this was suggested Duda pointed out that it’s often the case that we might accept a certain amount of technical debt in order to get the application out there.

While he is right and this is quite an unusual situation, we did see a similar situation on the last project I worked on.

On that project there was quite a tight delivery deadline for the first release so we knowingly incurred some technical debt in order to make sure that we met that date.

I’ve written previously about some of the technical debt that we incurred in that first release and while I think most of the time we made the right call I think there were still some occasions when we thought we were taking on deliberate prudent debt but were actually taking on deliberate imprudent debt.

Luckily it didn’t really come back to bite us and in the second release we had a much more relaxed pace and were therefore able to go through the code base and refactor certain parts of it to make it more maintainable.

J.B. Rainsberger has a really cool analogy about refactoring where he talks about cleaning the kitchen and cleaning the garage.

Cleaning the kitchen is what we endeavour to do all the time such that we’ll write a bit of code and then clean up after ourselves. Sometimes we don’t clean up enough and we end up with a bit of a mess which takes much longer to clean up – i.e. we need to clean the garage.

I think we sometimes drift towards thinking that we don’t need to clean the kitchen so often and end up cleaning the garage too often as a result.

This is something that Uncle Bob covered in a post he wrote around a year ago where where he points out that we’re more likely to take on technical debt when we didn’t need to rather than the other way around.

Finding the right balance between cleaning the kitchen and cleaning the garage seems to be something that comes from the experience of taking either approach too far.

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.

A couple of months ago I wrote about an approach to stories that Christian has been encouraging on our project whereby we slim stories down to allow us to deliver the core functionality of the application as quickly as possible.

In our case we had a requirement to setup a range of different parameters used to lookup reference data used in the different calculations that we have in our application.

At the time we created an interface that the rest of the application would interact with so that we could easily substitute the real version in when we needed to:

We released the first version of the application about a month ago and finally last week implemented the story where the data would move from being in memory to being in a database table.

One of the requirements which we had delayed by only having these parameters in memory was the ability to easily modify them.

Any changes that needed to be made required an update of the code and then redeployment whereas with the database approach we would only have needed to deploy a delta script.

In the event there has only been one occasion so far where those parameters needed to be updated so it hasn’t proved to be a problem.

Discussing this with a colleague on Friday he pointed out that what we’d done originally was to accept technical debt in our solution knowing that at some stage in the future we would need to address that.

The interesting thing about this case was that we knew exactly when we were going to repay that debt whereas it’s often the case that we create technical debt in a code base and vaguely know that at some stage we’ll address it.

As Martin Fowler points out in his entry on technical debt:

Just as a business incurs some debt to take advantage of a market opportunity developers may incur technical debt to hit an important deadline.

We probably saved at least a day’s worth of effort by delaying this decision and were able to work on functionality that the business needed by the deadline instead. We then paid back that debt last week when we had more slack in the system.

The benefits of getting something into the market place quickly are much greater than I previously imagined and I think we can look at our assumptions of how a solution ‘needs’ to be designed much more closely to see if we can make these trade offs more frequently.

Dermot and I spent a bit of time today refactoring some code where the logic had ended up in the wrong place.

The code originally looked a bit like this:

public class LookupService
{
	public LookUp Find(UserData userData)
	{
		var param1 = GetParam1From(userData);
		var param2 = GetParam2From(userData);
		var param3 = GetParam3From(userData);
 
		var lookupKey = new LookUpKey(param1, param2, param3);
		return lookupRepository.Find(lookupKey);
	}
}

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;
	}
}

We wanted to push the logic used to create the params in ‘LookUpKey’ into the ‘LookUpKey’ since it seemed that was a more natural place for that behaviour to live.

Unfortunately there were also a few other places that were use LookUpKey’s constructor so we couldn’t just go and change that to take in a ‘UserData’ otherwise we’d break other places in the code base.

My initial thought was that we could overload the constructor and temporarily have two constructors until we got rid of the original.

Dermot pointed out a better approach which was to add a static factory method to ‘LookUpKey’ and initially push the logic into that method.

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

The service is now much simplified:

public class LookupService
{
	public LookUp Find(UserData userData)
	{
		var lookupKey = LookUpKey.CreateFrom(userData);
		return lookupRepository.Find(lookupKey);
	}
}

We can also move tests that were originally indirectly testing the key creation by checking what was passed to the repository to be directly against the ‘LookUpKey’.

Eventually we want to make LookUpKey’s constructor private but for now we’ve been able to make our change without breaking the code base elsewhere.

On several of the projects that I’ve worked on over the last couple of years we’ve seen the following situation evolve:

• The team starts coding the application.
• At some stage there is a breakthrough in understanding and a chance to really improve the code.
• However the deadline is tight and we wouldn’t see a return within the time left if we refactored the code now
• The team keeps on going with the old approach
• The project ends up going on for longer than the original deadline
• It’s now much more work to move towards the new solution

In the situations I describe the refactorings could have been done incrementally but doing that would take longer than continuing with the original approach and also leave the code in an inconsistent state.

I think the reason this situation evolves consistently in this manner is because although we talk about writing maintainable code, delivery is often considered more important. Pushing out a delivery date in order to refactor code so that it will be easier to work with in the future isn’t something that I’ve seen happen.

Pushing a delivery date out is a cost that we can see straight away.

On the other hand it’s quite difficult to estimate how much of a gain you’ll get by refactoring to a more maintainable/easier to test solution and that gain will not be immediate.

We therefore end up in the situation where we tend to make major refactorings only if we’re going to see a benefit from doing that refactoring before the project ends.

In one sense that seems reasonable because we’re trying to ensure that we’re adding as much value as possible while the client is paying for our time.

On the other hand we’re making life harder for future maintainers of the code base which may in fact be us!

I’d be keen to hear how others handle these types of situations because it feels like this trade off is quite a common one and the way we’ve dealt with it doesn’t seem optimal.

Jeremy Miller tweeted earlier in the week about the dangers of using an auto mocking container and how it can encourage sloppy design:

That whole “Auto Mocking Containers encourage sloppy design” meme that I blew off last week? Seeing an example in our code.

I haven’t used an auto mocking container but it seems to me that although that type of tool might be useful for reducing the amount of code we have to write in our tests it also hides the actual problem that we have – an object has too many dependencies.

By hiding the creation of stubs/mocks for those dependencies in our test we are addressing the effect and not the cause i.e. we are bear shaving.

Jeremy followed this up with a couple of quite insightful comments:

You know though, I still think it comes down to you being responsible for paying attention.

It’s no different than still having to worry about DB optimization even though the ORM is shielding you from the details

Another somewhat related situation where I’ve noticed a similar problem is when we have several tests which require a certain method to be stubbed out and in the interests of reducing duplication we pull that up into a setup method.

While this achieves that goal it also means that there is information that is hidden away from us when we read each of our tests.

One approach that I’ve seen encouraged is that we should never use a setup method so that we have to create everything we need for our test in the test body.

I quite like this approach because it encourages us to see any problems that we’re creating with respect to writing difficult to test code but quite often what ends up happening is we’ll copy and paste tests because there’s more code to write for each test.

I’m coming to the conclusion that there’s no one approach that will stop us making design mistakes and that as Jeremy says, we need to make sure that we pay attention to what we’re doing.

I wrote a post a while ago about keeping consistency in the code base where I covered some of the reasons that you might want to rewrite parts of a code base and the potential impact of those changes but an interesting side to this discussion which I didn’t cover that much but which seems to play a big role is the role of incremental refactoring.

In our code base we recently realised that the naming of the fields in some parts of a form don’t really make sense and I wanted to start naming new fields with the new naming style and then go back and change the existing ones incrementally when it was a good time to do so.

Richard and Christian suggested that this wouldn’t be such a good approach because it would make the naming issue even more confusing until all the fields had been renamed.

In order to avoid this problem we had to either go and change every single field to follow the new naming approach immediately or settle on the old names even though they might not be as descriptive.

Since doing the former would involve changing the names of around 15-20 fields across several different objects, in Hibernate mapping code, probably in the database, on HTML forms and in Watin tests we decided not to do that – the project is only for a short amount of time so the investment probably wouldn’t be worthwhile.

Although in this case it makes sense not to make the improvement it doesn’t strike me as being entirely satisfactory that we would need to make this type of change in a big bang fashion.

From my experience there are often insights into the code or improvements in the ubiquitous language as time goes on and while consistency is of course an important thing in any code base it’s not the only thing.

When do we decide that actually gradually moving to a better approach is worth the temporary pain that having this inconsistency will cause?

Although I’m generally quite opposed to coding approaches whereby we mutate objects, sometimes the way a framework is designed seems to make this a preferable option.

We came across a situation like this last week when we wanted to hydrate an object with data coming back from the browser.

The signature of the action in question looked like this:

public class SomeController
{
	public ActionResult SomeAction(string id, UserData userData)
	{
	}

We were able to automatically bind most of the values onto ‘UserData’ except for the ‘id’ which was coming in from the URL.

We could have included the ‘id’ in a hidden field in the page to get around this problem but that seemed like a more complicated/hacky solution.

Instead what we needed to do is the following:

public class SomeController
{
	public ActionResult SomeAction(string id, UserData userData)
	{
		userData.Id = id.ParseIntoId();
	}

‘ParseIntoId’ is an extension method which converts a string representation of an ‘id’ into an object representation.

While this method is fine as it is, I find that as code gets added it can sometimes be less than obvious that we’ve actually mutated ‘UserData’ so I now prefer to explicitly call that out like so:

public class SomeController
{
	public ActionResult SomeAction(string id, UserData userData)
	{
		var userDataWithId = UpdateUserDataWithId(userData, id);
	}
 
	private UserData UpdateUserDataWithId(UserData userData, string id)
	{
		userData.Id = id.ParseIntoId();
		return userData;
	}

We would then want to use ‘userDataWithId’ after this point in the function.

Of course the original ‘UserData’ has still been mutated and it doesn’t seem worth the extra code that it would take to avoid this so we’ll just trust that anyone reading the code would realise that they should use ‘userDataWithId’ instead!

Reg Braithwaite has a cool series of posts where he covers the different combinators from Raymond Smullyan’s ‘To Mock a Mockingbird’ book and one of my favourites is the ‘Kestrel’ or ‘K Combinator’ which describes a function that returns a constant function.

It’s described like so:

Kxy = x

The Kestrel function would take in 2 arguments and return the value of the first one. The second argument would probably be a function that takes in the first argument and then performs some side effects with that value.

Braithwaite descirbes the ‘returning’ function from Rails as an example of this combinator whereby instead of writing code like this:

def registered_person(params = {})
  person = Person.new(params.merge(:registered => true))
  Registry.register(person)
  person.send_email_notification
  person
end

We can write this:

def registered_person(params = {})
  returning Person.new(params.merge(:registered => true)) do |person|
    Registry.register(person)
    person.send_email_notification
  end
end

i.e. we can group all the side effects together and it’s more obvious to the reader that we’re returning the value of ‘Person.new(…)’ from this method.

I’ve been writing a bit of code in F# to generate some test objects and I realised that I had code like this in a few places:

let build (t:Type) =
      let theType = Activator.CreateInstance(t)
      theType.GetType().GetProperties() |> Array.iter (fun p -> p.SetValue(t, createValueFor p, null))
      theType

We’re creating ‘theType’ and then mutating it straight away using reflection before returning the value.

We can implement a ‘returning’ function like so to simplify the code a little:

let returning t f = f(t); t

let build (t:Type) =
    returning (Activator.CreateInstance(t)) (fun t -> 
        t.GetType().GetProperties() |> Array.iter (fun p -> p.SetValue(t, createValueFor p, null)))

I think this is the same as what Martin Fowler refers to as a nested closure and it seems quite a neat way of encapsulating side effects and making the code a bit more expressive.

I’ve previously written about the value of adding duplication to code before removing it and we had an interesting situation this week where we failed to do that and ended up generalising a piece of code too early to the point where it actually didn’t solve the problem anymore.

The problem we were trying to solve was around the validation of some dependent fields and to start with we had this requirement:

Given 2 fields...
 
Field 1
Field 2 - Can be empty if Field 1 is 'Foo'. Otherwise must have a value.

We wrote an object that looked a bit like this to do that logic:

public class DependentOnField1 : IValidateFields
{
	public bool IsSatisfied(Field field)
	{
		if(OtherFieldIsFoo())
		{
			return true;
		}		
 
		return string.IsNullOrEmpty(field.Value);
	}
}

Later on we had the following requirement:

Given 2 more fields...
 
Field 3 
Field 4 - Must have a value of more than 0 if Field 3 is 'Bar'. Otherwise can be empty.

It seemed at first glance that they were the same type of validation because in both cases there is talk of dependence between fields.

We started refactoring the ‘DependentOnField1′ object into a state where we would be able to use it for both cases. This meant that we needed to parameterise the object so that we could vary the dependent field.

We eventually ended up with an interface that could be called like this:

var field1DependenceValidator = DependentOn.Field(field1);

public class DependentOn : IValidateFields
{
	public static DependentOn Field(Field field)
	{
		return new DependentOn(field);
	}
}

Unfortunately while refactoring this object we hadn’t taken into account the fact that the requirement for fields 3 and 4 is slightly different to that for fields 1 and 2. With fields 1 & 2 we only needed one condition to be true whereas with fields 3 and 4 two conditions needed to be true – field 3 must be ‘Bar’ AND field 4 must be greater than 0.

We initially tried to hack in the fields 3/4 requirement like this:

public class DependentOn : IValidateFields
{
	public bool IsSatisfied(Field field)
	{		
		if(OtherFieldIsX() && Double.Parse(field.Value) > 0)
		{
			return true;
		}		
 
		return string.IsNullOrEmpty(field.Value);
	}
}

Unfortunately that breaks the way that validation works for fields 1 & 2 and every other attempt we made seemed to make the situation worse and worse until after about half an hour of wrestling with this we decided to revert everything and just create a new object for this new type of validation.

We would accept some duplication until we were able to see a more meaningful abstraction that we could pull out.

Dave Cameron, who first taught me this approach, recently wrote a post describing how he removed some duplication in JavaScript code and you can see from the initial example in his post that he waited until he was exactly sure where the duplication was before getting rid of it.

I think this is the best approach and the rule of thumb seems to be that we should wait until we have the same thing 3 times before trying to remove the duplication.