Shotgun surgery
Possibly my favourite code smell
What’s your favourite code smell?
Daft question, I hope you’ll agree.
But I think I do have a favourite. It’s Shotgun Surgery. Why? Because it tells me so much about my design. Let me explain…
Firstly, many of the other code smells often feel quite ambiguous to me. Is this Feature Envy or is it a reasonable separation of responsibilities? Is this Primitive Obsession or simple value passing between close collaborators deep inside an encapsulated mechanism? Very often I spend too much energy debating questions like this, instead of making progress towards more habitable code.
But Shotgun Surgery is different. It says “You just added feature X and you had to change code at A, B and C”. This means that A, B, and C were coupled — and are now coupled even more.
Note the use of the past tense there. I don’t want to look for or fix Shotgun Surgery until the feature1 increment is done. I want to wait so that I can see all of the damage I’m going to do, because that tells me the most about what the design problem was.
Shotgun Surgery says “You ought to refactor so that you could have implemented that feature increment with a 1-line change to existing code”.
So here’s the process:
Add a complete feature increment.
Step back and review all of the places in the code that had to change. There was implicit coupling between all of those places, because they all made some assumptions that had to be busted in light of our new feature.
Construct a design that would have allowed the feature to be added by instead changing only one place in the code.
Refactor to that design.
Why would I want to do this? Because when I’m done, my code will now have a new “seam” — a new place at which extensions such as this can be inserted without changing too much.
Imagine that my code currently has the following design:
Without too much fuss I implement my feature increment. And now I have this (changes in red):
What if, instead, I had first refactored my design to make room for the new feature:
So that I can now implement my feature with little or no change to existing code (new feature in green):
Of course, I’m not claiming that any of this is easy, but I do think it’s valuable. That’s because fixing the Shotgun Surgery in this way prepares the codebase for the future in a number of ways:
The code for this whole feature is now collected together in one place, and likely has a name. That’s got to be good for our ability to comprehend the code: It is now obvious where to look for anything related to this feature.
The fact that we were asked to add this feature to an existing, working application suggests that similar changes will be asked for in the future [citation needed]. By taking this “open-closed” approach we have likely made the code more amenable to adding alternative options along the same axis.
Even if nothing similar crops up again in the future, by making this change we have ensured that the rest of the codebase isn’t infected with knowledge of this feature. So the rest of the codebase is cleaner and not burdened with this responsibility.
Deleting this feature will be really easy, if it comes to that.
There’s likely little or no implicit coupling related to this feature now. So there will be fewer surprises if it evolves.
The part of the codebase that had to be modified to accommodate this feature is now a more stable abstraction, parameterised by the new feature and its alternatives. And the more we do this, the more generalised and stable the core of our codebase will become. This is expected to be good for the long term extensibility.
It seems to me that the benefits to the code of fixing Shotgun Surgery like this are huge and long-lived. But there are also benefits in terms of our mindset: making this change will influence how we make future changes, and how we structure our design to support future change.
Feels like I should write up a worked example someday…
Things to try
Have a go at Matteo Vaccari’s OCP Kata, which was the inspiration for many of the above ideas. This kata tells you to refactor in this way every time you have a failing test.
When you finish your next feature increment, do the design exercise above: work out a design in which all of the feature’s code could have been added in one place.
If you have time, actually change the code to that design.
Try do the kata on a “real” production codebase, refactoring on RED so that you can make the next test pass without changing code. Which works best for you, bigger steps or smaller steps?
As always, please leave a comment if you try this exercise. Does the codebase feel substantively different afterwards? Could you have implemented the feature like this in the first place? Will you do anything differently for your next feature?
What do I mean by “feature” in this article? I guess it’s anything that is big enough to be asked for by your Product Owner, and therefore also releasable to users.