I was re-reading some of Ted's stuff today, and came across his post on refactoring nested if statements. It struck a cord, because I'd just teased apart a nested for loop - if statement combination recently, mainly for the reasons Ted states:
When I see multiple nested ifs in a method, it seems to indicate a problem with responsibility. Usually, you are asking one object one thing, another object another thing, then even another object a third thing and then finally you operated on that third thing.
I've gained the ability to see these types of situations since I started on this project, and the one thing that really stands out when we look at nested ifs and breakdowns in object responsibility is that they make things really hard to work with. When we tease out an if into a separate method, we gain additional descriptors about what the code is trying to accomplish. For example (pseudo code in a class named MeasurementPoint):
public void createDummyOverriddenAllocationResults(
OwnershipType ownershipType){
List dummyResults = new ArrayList();
List overriddenAllocResults = getAllOverriddenBacars;
List facilities = getAllProductionSourceFacilities();
foreach(Facility fac in facilities){
if(facility.canBeOverridden(ownershipType())){
List allProducers = facility.getAllProducers();
foreach(BusinessAssociate ba in allProducers){
if(ba.hasVolumeBeenOverridden(
overriddenAllocResults, ownershipType)){
dummyResults.add(createDummyAllocationResult(ba, ownershipType, etc));
}
}
}
}
return dummyResults;
}
That's the basic gist of what I was dealing with, although the actual code was a little more "wordy". So we can see from the name of the method that we're attempting to create "dummy" overridden allocation results. That's nice, but how does the method actually decide how to create these dummy records. The answer is not immediately obvious without spending a good chunk of time inspecting the code. Whenever I see something like this I'm tempted to at least break the method down into smaller methods that help document what I'm trying to do. So the above method becomes more like this:
public List createDummyOverriddenAllocationResults(
OwnershipType ownershipType){
List overriddenAllocResults = getAllOverriddenBacars;
List facilities = getAllProductionSourceFacilities();
return createZeroVolumeOverriddenResultsForProducersWithNoVolumes(
overriddenResults, facilities);
}
So all we've done here is move the first foreach loop into its own method, that gives us a little more insight into how we're accomplishing our task. But now we've just moved the problem into another location. So let's break it down further:
public List createZeroVolumeOverriddenResultsEtc(
List results, List facilities){
List dummyResults = new ArrayList();
foreach(Facility fac in facilities){
List producers = fac.getProducersWithoutOverrides();
dummyResults.addAll(createOverrides(producers, etc));
}
return dummyResults;
}
We keep this up until we have all the interesting pieces wrapped up in small, do-only-one-thing methods that document what they're doing with their names and parameters. One bad smell is really long method names... if you can't describe what your method is doing without hitting the line-break in your code editor you can probably decompose even further.
A nice side-effect of the above refactoring was that I was able to override one of the methods in a sub-class to make a story work. In essence, after the refactoring, the sum-total of the story turned out to be a one liner (at least in the domain, the gui was another beast...).