We’re on a path more like re-implementing the program than refactoring it. That’s a concern. Can we do better?
We have a plan and a path for improving this program. We have the notion of a “wrapper” that enfolds each different
Item type, and that knows how to update it. We’ve used that pattern to implement our new change, and have built a wrapper for all other types that just applies the old code for all the old types. Now we have some options.
First, and in some ways perhaps the wisest, we are in good shape for all new types of item. We TDD the wrapper for that item into being and apply it. That item will remain independent of any other items and we can be confident that it works and no other item’s code can hurt it. You could make a darn good case that we should stop now.
Even if one of the older item types changes, we don’t have to edit the old code. We can create a new wrapper for that type, TDDing it until we’re confident. That removes it from the clutches of the old code. Over time, maybe the old code becomes unused. Maybe it just stays there being ignored. That might be just fine, and it certainly minimizes work.
Second, the path we’re on: incrementally TDD the old items into wrappers, using the spec and considering the old code. We need to look at both spec and code, because sometimes people change the code in response to a request, but don’t update the documents. No, really, it does happen.
There is a legitimate concern with this approach. The spec could be wrong, and the code is hard to understand. We stand additional risk of errors. Between the extra work and the added chance of error, our current path is less desirable on the face of it, than the first.
Refactor the Old Method
What if it were possible to improve the code in the old method by real refactorings, transformations on the code that were guaranteed not to change its function? We did one such when we inverted the sense of an if statement. That can’t change the results (assuming we don’t mess up the inversion). Some IDEs will do that refactoring automatically, which gives us great confidence. My editor can’t do that, but I’m pretty good at converting a != to an ==, so I’m quite sure I did it right. Plus we have tests.
If we were going to write a procedural version of the Gilded Rose requirements, a switch-case statement or a series of if-elsif would probably be where we’d go:
if it is this_kind update sell_in this_way update quality this_way elsif it is that_kind update sell_in that_way update quality that_way elsif ... end
If we wanted a structure more like the one we’re moving toward now, and we had that sort of update method, the refactoring to wrappers or pluggable functions or whatever would be straightforward. But with what we have now, it’s whatever the opposite of straightforward is. Twistybackward?
Aaanyway … if we could refactor all the way to a structure much like the if-elsif one, then we could continue to move toward our current wrapper approach if we wanted to.
What Shall We Do?
Truth is, I think the wise thing to do is to stop now. There is no business purpose for improving the existing code. We have it isolated and we will never need to edit it, because whenever a new requirement comes up, we can simply build its code in a new wrapper. (We might at that time want to put a check into the old method to be sure that type never shows up, but that would be a belt-suspenders kind of move.)
I’m tempted to try unwinding the method, more as an exploration of technique than as a suggested good idea. We’ve isolated this code, encysted it, and it’s rendered harmless. But I’d really like to know if it can be converted in a refactoring kind of way, into decent code.
I’m going to take this article to a sort of conclusion, and then if the fancy strikes me, try refactoring the monster.
In Michael Feathers’ excellent book, Working Effectively with Legacy Code, he describes a pattern called “Strangler”, where you remove bits of legacy code, replacing it with code under test, until slowly the old code is “strangled”, no longer in use. We’re standing on the threshold of that pattern here: over time, less and less use will be made of our old method, which is itself nicely isolated.
I’ve reflected often with the fate of the famous / infamous C3 project, the first Extreme Programming project, the Chrysler Comprehensive Compensation effort. We took our job as replacing the existing Chrysler payroll application, which was a mess of COBOL code. We thought we had to implement everything. And we did it.
However, the winds of politics, and perhaps even some good reasons, resulted in the C3 code being abandoned and replaced at incredible expense by a commercial payroll app. That made us sad and has been used to call the principles of Agile and XP into question.
What we should have done, in retrospect, and in my opinion, was to replace parts of the existing payroll, with whatever form of program was suitable to that part. We should have applied Strangler. I believe that if we had done that, much of our code would still be running today, and the expenditure of millions of dollars on yet another payroll program might have been avoided.
We’ll never know, but the lesson stands out in my mind.
Historically I’ve undertaken a number of efforts to completely replace some existing application. To my recollection, none of those efforts was ever really successful, even the ones that ultimately shipped.
The result we have here, for Gilded Rose, is pretty close to just right. We have a structure that will readily support new items, and that will support changes to pricing for existing ones.
We might want to beef up the ridiculous testing for exact product names instead of key strings, but beyond that, I think this thing is nearly good enough.
We should stop, just about right here. I’m kind of surprised at this result. It isn’t what I planned to conclude when I started this article. But there it is.