Mark Needham

Thoughts on Software Development

Incremental Refactoring: Create factory method

with 7 comments

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.

Be Sociable, Share!

Written by Mark Needham

June 17th, 2010 at 12:43 am

  • http://blogs.sourceallies.com/author/sramasamy/ sudr

    I love the simple examples used to highlight everyday decisions that you have to make when coding.

    I don’t quite understand why the Factory method is a better solution than a second constructor that takes in UserData. I can understand that if you are going down the path of having many more constructors then a Factory may begin to make more sense. But if you only eventually need to construct a LookUpKey from a UserData object wouldn’t a constructor suffice?

  • http://www.markhneedham.com Mark Needham

    We want to drive the code to the stage where we only use the factory and not have the pubic constructor – I think it makes it more obvious that we’re constructing the ‘LookUpKey’ from the ‘UserData’ because the code reads ‘LookUpKey.CreateFrom(…)’ rather than ‘new LookUpKey(…)’.

    It also felt to me that the code was in a less inconsistent state if we did it this way and we didn’t have to push the logic used to derive ‘param1′, ‘param2′ into a constructor which I think would have been more misleading than the current solution.

    Perhaps that’s all just personal preference/opinion though and overloading the constructor would be an equally valid approach.

  • Mike wagg

    I like this approach a lot. In many of our classes creation is as interesting responsibility in itself. In these cases we are not doing our best by just adding another conductor. Using a factory method gives us a chance to make it clear that the construction of the object is interesting and to give it a name which will help future developers understand the intention of the method. Its much nicer to have a nicely named factory method than to have to pick between constructor overloads which give you little context.

  • Buckley

    Just a small note : in the sample the factory method isn’t static

  • http://www.markhneedham.com Mark Needham

    @Buckley – good spot, have fixed. Thanks for pointing it out.

  • http://symprise.net Rafael Peixoto de Azevedo

    Greetings, Mark!

    Where should we place the responsibility for creating the LookUpKey from UserData’s param1, param2 and param3?

    The post discusses three possibilities: 1) in the LookupService, 2) as a LookUpKey constructor and 3) as a LookUpKey factory method.

    In order to illustrate the rich diversity of design options that are open to us, I suggest an additional alternative.

    If the combination of param1, param2 and param3 represents a relevant concept for UserData, the LookUpKey class could be renamed after this concept and the creation responsibility would be better placed in the UserData class.

    Cheers, Rafael

  • Pingback: Coding: Having the design influenced by the ORM at Mark Needham