Jeff Perrin :: Archives for February, 2006
Saturday, February 25th, 2006
I’ve been in a refactoring kind of mood lately, so I guess I’ll continue with the theme in this post…
One of the major frustrations I have when it comes to working on my current project is that a lot of code has been written by a lot of different people. A constant battle we fight is to push logic down from the presentation layer back into the domain where it belongs, and can be tested. Often, I find myself weighing the benefits of refactoring a chunk of code versus just getting the required bit of functionality completed and moving a sticky to the “done” pile. Far more often than I’d like, I just sigh to myself internally and finish the feature or bugfix without taking further action (unlike Vlad, who immortalizes his resignation within the codebase).
Well, on Thursday I’d finally had enough. Right before 5:00, Vlad passed me the umpteenth bug filed on remote inventory functionality. I took a look at the bug, went to the offending JSP’s form helper class, and proceeded to sigh yet again.
For those individuals who aren’t lucky enough to be working on our project, our form helper classes are generally known as the dumping grounds for bad, untestable code. They are also unfortunately directly accessed by the JSP’s, and are untested within our system. They’ll generally be called by Struts actions like so:
public void save(){
FormHelper helper = operationsForm.getFormHelper();
helper.save();
helper.initialize();
forward("success");
}
That method looks nice. So what’s the problem? What does the helper.save() method look like?
public void save(){
Mpoint deliveryMpoint = delivery.getMpoint();
if(deliveryMpoint.hasRemoteInventory() == false){
mastersFacade.createRemoteMpoints(deliveryMpoint);
}
delivery = mastersFacade.updateMeasurement(delivery);
closingInventory.setVolume(
delivery.getVolume().subtract(
sale.getVolume().add(openingInventory.getVolume())));
closingInventory = mastersFacade.updateMeasurement(closingInventory);
sale = mastersFacade.updateMeasurement(sale);
}
Wow, no wonder we keep getting bugs on this stuff. Either this entire block of code is in every test of this feature, or (Chris was right on this one) it’s not actually tested! Our untested save method:
- Finds a measurement point
- Checks to see if it has remote measurement points. If it doesn’t, create some
- Saves the value of the delivery measurement (which can’t actually be edited on this screen anyways)
- Calculates and saves the value of the closing inventory measurement
- And finally saves the value of the sale measurement (which is the only editable field on the form)
Not so sexy. The “initialize” method looks much the same, making at least 4 separate facade calls. At this point, I actually proceeded to do something about it. Three hours and about 5 tests later, I’d managed to completely refactor the entire thing, turning the offending code from above into this:
public void save(){
mastersFacade.updateRemoteInventory(remoteInventoryMaintenanceDto);
}
All the logic has been pushed down to the domain (RemotableMeasurementPoint) where it belongs, and I actually feel like I’ve accomplished something. For the record, this was the first time I’ve really felt like I hit the “coding groove” for quite a while. It felt good.
Posted in Programming | No Comments »
Thursday, February 16th, 2006
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…).
Posted in Programming | No Comments »
Monday, February 13th, 2006
I first read Sam Smoot’s stuff in the architecture forum at asp.net, where he was one of the few individuals with substantial (heh) posts. Somewhere along the line Sam found the way and has transferred his commentary from the asp.net forums to his blog at Substantiality.net (get it?). It’s a site worth checking out, in my humble opinion.
Posted in Programming | No Comments »
Saturday, February 11th, 2006
I just discovered Smart Home.com the other day. A nerd could (did) spend hours on this site.
Posted in Tech | No Comments »
