I did a little bit of refactoring the other day that I'm pretty happy with, so I thought I'd share.
We started out with a method in the SystemProfile who's signature is specified below:
List createPriorityAllocationGroups(
ProfileGroup profileGroup,
List allAllocationChildren,
FacilityMonthlyMeasurements sourceForMeasurementPoints,
FacilityMonthlyMeasurements sourceForOwnerTypeClassification)
It is called within a loop for every ProfileGroup contained in the SystemProfile like so:
private List getProfileAllocationGroupsForProfileGroups(
List profileGroups,
List allAllocationChildren,
FacilityMonthlyMeasurements sourceForMeasurementPoints,
FacilityMonthlyMeasurements sourceForOwnerTypeClassification){
List profileAllocationGroups = new ArrayList();
for (Iterator iter = profileGroups.iterator(); iter.hasNext();) {
ProfileGroup profileGroup = (ProfileGroup) iter.next();
profileAllocationGroups.addAll(
createPriorityAllocationGroups(
profileGroup,
allAllocationChildren,
sourceForMeasurementPoints,
sourceForOwnerTypeClassification,
isCustomProfile));
}
sortProfileAllocationGroups(profileAllocationGroups);
return profileAllocationGroups;
}
Having to pass in four parameters to a method is a little smelly, but not too bad. The bad part happened within the method, which had grown quite complicated over a long period of time. The purpose of the method is to simply assemble a list of new PriorityAllocationGroup objects, so it should have looked something like this:
PriorityAllocationGroup allocationGroup = createNewPriorityAllocationGroup(group);
allocationGroup.addAllocationChildren(getPriorityAllocationChildrenForGroup(group));
allocationGroup.addMeasurementPoints(getMeasurementPointsForGroup(group));
return priorityAllocationGroup;
Instead, we had an implementation that was about 75 lines long, and so convoluted it took me and my pair almost half a day to figure out how to implement a feature for our story. Which sucks.
The very first refactoring I identified was to move the method into ProfileGroup. Many of the questions being asked within the method were specific to this object, so keeping it in SystemProfile no longer made sense. This involved moving a lot of other helper methods into ProfileGroup as well. Nothing actually got any simpler, but at least we were now just asking ProfileGroup to assemble it's ProfileAllocationGroups instead of stuffing it all in SystemProfile.
So originally we had this:
class SystemProfile{
List getProfileAllocationGroupsForProfileGroups(
List profileGroups,
List allAllocationChildren,
FacilityMonthlyMeasurements sourceForMeasurementPoints,
FacilityMonthlyMeasurements sourceForOwnerTypeClassification);
List createPriorityAllocationGroups(
ProfileGroup profileGroup,
List allAllocationChildren,
FacilityMonthlyMeasurements sourceForMeasurementPoints,
FacilityMonthlyMeasurements sourceForOwnerTypeClassification);
}
Which became this:
class SystemProfile{
List getProfileAllocationGroupsForProfileGroups(
List profileGroups,
List allAllocationChildren,
FacilityMonthlyMeasurements sourceForMeasurementPoints,
FacilityMonthlyMeasurements sourceForOwnerTypeClassification);
}
class ProfileGroup{
List createPriorityAllocationGroups(
SystemProfile systemProfile,
List allAllocationChildren,
FacilityMonthlyMeasurements sourceForMeasurementPoints,
FacilityMonthlyMeasurements sourceForOwnerTypeClassification);
}
The next refactoring involved removing another smell (at least to me); passing in Lists as parameters. Obviously there are sometimes reasons for this, but it's usually better to be able to call a method like getAllAllocationChildren() at will instead of passing the list all over the place. As it turned out, the allAllocationChildren parameter originated from a method within SystemProfile. Since we were passing the SystemProfile in as a parameter, we could eliminate this parameter to make the method signature look like this:
class ProfileGroup{
List createPriorityAllocationGroups(
SystemProfile systemProfile,
FacilityMonthlyMeasurements sourceForMeasurementPoints,
FacilityMonthlyMeasurements sourceForOwnerTypeClassification);
}
One of the patterns I saw after this cleanup was that the 4 parameters originally passed in were in turn being passed in to other methods. Over and over again. Eventually the lightbulb goes on in my head as I realize that what we're actually signifying by passing these parameters all over the place is that we're operating inside a specific context. So the next refactoring was to combine the sourceForMeasurementPoints, sourceForOwnerTypeClassification, and the SystemProfile into a ProfileAllocationGroupContext object. Once this was done, it was a simple matter to change the signature of all the helper methods to take the context object as well.
class ProfileGroup{
List createPriorityAllocationGroups(
ProfileAllocationGroupContext context);
}
It's now the job of the SystemProfile to create the ProfileAllocationGroupContext, which is then passed on to every ProfileGroup when we ask for its ProfileAllocationGroups. Whew!
This seemed ok, but there was still some stuff that didn't quit fit. There were lots of calls like this in ProfileGroup.createPriorityAllocationGroups:
context.getMeasurementPointsToBeAllocated(this);
This was a fantastic example of how when you're refactoring code it sometimes "speaks" to you. I had originally thought that putting this code in ProfileGroup was the right thing to do, but as I worked through all the above refactorings it became obvious that it wasn't. Here's the method that kicked everything off in SystemProfile as it stood at this point in the refactoring:
List getProfileAllocationGroupsForProfileGroups(
List profileGroups,
ProfileGroupAllocationContext context){
List profileAllocationGroups = new ArrayList();
for (Iterator iter = profileGroups.iterator(); iter.hasNext();) {
ProfileGroup profileGroup = (ProfileGroup) iter.next();
profileAllocationGroups.addAll(profileGroup.createPriorityAllocationGroups(context));
}
sortProfileAllocationGroups(profileAllocationGroups);
return profileAllocationGroups;
}
The code was telling me that instead of
profileGroup.createPriorityAllocationGroups(context)
we should actually be doing
context.createPriorityAllocationGroups(profileGroup)
This makes sense, if we say that for the given context, we want to assemble a list of PriorityAllocationGroups. By moving the method to the context object we're actually making it do double duty as an assembler or builder, but that can easily be factored out later. So in the end, my object model looked like this:
class SystemProfile{
List getProfileAllocationGroupsForProfileGroups(
ProfileAllocationGroupContext context);
//plus a bunch of state and other methods
}
class ProfileGroup{
//original state based stuff
}
class ProfileAllocationGroupContext{
List createPriorityAllocationGroups(ProfileGroup profileGroup);
}