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 — reasonablly 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.
marc
July 24, 2009
The key thing I learned here was your admission of not noticing the degradation early. I’m the opposite, I sometimes think about the degradation too early. I sometimes catch myself, when talking to the business, simultaneously wondering which classes are most at risk!
What I learned was not that we are all different — surprise! — but how far off the radar this can be to an experienced prog. It certainly explains the blank looks I get sometimes when I ask, “Why are you putting that there?”
Kevin Rutherford
July 24, 2009
I think I noticed most of the components of this problem quite early. But I saw them as only localized problems; I hadn’t put together the bigger picture until the code screamed at me.
miluch
March 2, 2010
From my perspective the hardest changes are these that spans over every manager/service of your system. In your case you were able to create a single point in your codebase that is responsible for the change and possibly for similar changes as well.
But imagine situation that you are introducing a new kind of users to your system or even worse 2 new kinds of users. The UI layer stays the same but majority of bussiness rules/validations differs from kind to kind. You could end up having a “user kind” switch statement all over all your system, since depending on user kind the logic of your system is different…
How to cope with these kind of changes???
Kevin Rutherford
March 3, 2010
I’d deal with it in exactly the same way, although it might require quite a bit of refactoring to get there. Those switch statements are an indication that the code has a missing abstraction — possibly a User class hierarchy? Each branch of each of those switches wants to be on some kind of user object.
Martin Fowler gives explicit advice on how to deal with the Switch Statements smell in his ‘Refactoring’ book (and so do I in ‘Refactoring in Ruby’, where we renamed the smell to Simulated Polymorphism). The general idea is that by a series of refactorings you’ll create that missing class hierarchy; it is then often quite easy to add a new type of user, simply by adding a new subclass.
And if you only have one kind of user right now, perhaps the simplest plan would be to add the second type by “brute force” means, thus creating a whole bunch of smelly switch statements, and then proceed as above. That is, introduce the smell, and then follow the recipe to fix it.
miluch
March 3, 2010
Hi
Thanks for a response. I am reading Fowler’s Refactoring book and i think i know how to tackle with these kind of switch statements (replacing type code with subclass or strategy/state). It could work in some scenarios but actually i can not imagine how this kind of refactoring can be employed for such a big change in TYPICAL 3 layered applications.
If you decided to create hierarchy for User and than tried to transform every “user type” switch statement to separate abstract method on User you would end up having more than 100 of these kind of methods !
Subclasses of User to fulfill the contract would delegate to other classes ( managers/services classes) but at the end you would get User class as a GOD class/facade.
The user class would have not only “business” logic but also some kind of VIEW logic .i.e. some jsp pages needs to present additional widgets if user is of certain type.
On the other side i am thinking of employing a different approach: create single hierarchy for each service/manager to represent different behavior for each user type.
Adding new user types would be harder since i would have to define a multiple new service/manager implementations but this time no GOD class would be created.
Kevin Rutherford
March 3, 2010
Well, if most of the switch statements test on the user type i would still create the User hierarchy, and then solve the other problems later. It is always tempting to try to imagine what the code will look like after a series of refactorings, but I usually find I get it completely wrong — I can’t visualise complex changes to large amounts of code, so I just fix the smells that’s right under my nose and then see what’s next.
miluch
March 11, 2010
Following your advice to create single User hierarchy to encapsulate user-type dependent behavior makes sense, moreover it would easily allow me to add new user type in a future (just implement single abstract class/interface).
As i stated above all these terrible switches are on userType so single abstract class User with single method for each user-type-switched functionality seems to be natural approach to this kind of problems.
On the other side i am not a fan of creating classes (even abstract classes) with more than 8-10 methods. In this case abstract class User would have more than 100 methods. Moreover( this is what hurts me more), these methods would be both business and UI related,.i.e User class would have method like calculateDiscount(…), getWorkGroups() but also methods like isAddressValid(…), showWidgetXonPageY() and more.
Mixing UI and business concepts in a single class does not seem to be OK.
Kevin Rutherford
March 11, 2010
You have over 100 switch statements testing the user type? Wow. I would still go ahead and make the change I suggested; then I would look for cases of Feature Envy, where several of these user methods are called together (start with the obvious violations of Tell-Don’t-Ask such as getWorkGroups(), for example). Beyond that it’s hard to say without seeing code, but it sounds as if there is likely to be plenty of opportunity to split out helper classes. Interested to hear how it goes.