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.