I’ve just finished refactoring the code of Reek to (almost) eliminate a large dose of Shotgun Surgery, so I thought the time was ripe for a little reflection on what occurred and why.
What is Shotgun Surgery?
This is a code smell that occurs when you have to change loads of existing code in order to make a single change to the overall codebase.
In this case I wanted to change the reports generated by Reek’s spec matchers so that test failures are described more succinctly. To do so I had to change 7 classes, adding 4 methods (three of which all had the same name) and modifying several others. I pushed quickly to GREEN, at which point the code looked like I had driven a bulldozer through it from one end to the other and back again.
Ok, that happens all the time. Why is it a problem?
Because it creates drag. I had known for a while that certain kinds of change in this codebase would be painful to make, and the pain I imagined was subconsciously steering me away from making those kinds of change. I had been spending my time making changes that I knew were easier, putting off tackling the no-go areas of the codebase.
Furthermore, as I was tackling this smell I discovered a few defects in the code. They were edge cases, but they were definitely bugs. They had festered there in the dark corners where I had been afraid to go; and for all I know they had tripped up some new user of Reek and put them off forever.
And perhaps worst of all, it indicates that there’s a single design decision whose implementation (and consequences) is distributed throughout the codebase. So any similar or related decisions are going to be just as difficult and messy, and pretty soon they’ll all pile up together and render the code completely unmaintainable.
Why did it get so bad?
I think the main reason is that I’m not particularly well tuned to noticing Shotgun Surgery. (I believe I’m not alone in this.) We write a test, and it takes a bulldozer to get to green. We get to green, and we make a few localized refactorings to tidy up some of the methods we touched. But we often don’t see the bigger picture. “Hey, I just had to touch 6 classes to get that done; there’s a design issue here.” I’m sure I had previously failed to notice this or related smells on several previous occasions.
I had also failed to listen to my inner demon. I had “seen” several pieces of the eventual puzzle many weeks earlier, but I hadn’t addressed them because they didn’t seem serious. For example, I knew that some classes violated one of my central tenets of a good hexagonal architecture. But I told myself that was just a guideline — I shouldn’t push the code towards that shape unless the code told me it wanted to go. It turned out that the code had been screaming at me for some time!
How did I fix it?
The first, and most important, step is to identify the decision that hadn’t been properly and uniquely represented in the code. The aim is to refactor the code so that if I wished to change that decision again I would only need to make a single change to a single class. This is the hardest part of fixing Shotgun Surgery, and it’s a design step.
In this case I wanted to create a different kind of report, and I wanted to do it in the spec matcher. But quiet reports were currently a special case within full reports, implemented by reading an attribute from a global variable containing the user’s preferences. And the report itself was created way over the other side of the system. In a lazily evaluated cache.
I won’t bore you with the details. I decided to do this one “by the book”, inverting dependencies one by one until the Report object was created once, at the identified decision point in the code. Over the course of a dozen hours I changed all of the above 7 classes again; I introduced 4 new classes; and at the close of play none of the methods I had touched in the original change still remained.
Could this cost have been avoided?
I think this is a natural process — reasonably well-refactored code suddenly receives a change request that goes against the grain, so there’s some origami required before the old and the new sit comfortably together. That much is inevitable.
But I’m sure I missed fainter warning signs earlier in the code’s life. Had I responded to them, this week’s change might have been much less traumatic. So in future I will try to be much more vigilant: the next time a test causes me to change more than one class, I’ll sort it out right there and then.