This is part 2 of a small series about code smells and possible refactorings. The target audience I had in mind are newbies who heard about this topic and who maybe wanted to wait a bit before entering these advanced waters. The following article covers “Feature Envy”, “Shotgun Surgery” and “Divergent Change”.

Topics

• Feature Envy
• Shotgun Surgery
• Divergent Change

What you’ll quickly realize with code smells is that some of them are very close cousins. Even their refactorings are sometimes related—for example, Inline Class and Extract Class are not that different.

With inlining a class, for example, you extract the whole class while you get rid of the original one. So kinda extract class with a little twist. The point I’m trying to make is that you shouldn’t feel overwhelmed by the number of smells and refactorings, and certainly shouldn’t get discouraged by their clever names. Things like Shotgun Surgery, Feature Envy and Divergent Change might sound fancy and intimidating to people who just got started. Maybe I’m wrong, of course.

If you dive a little into this whole topic and play with a couple of refactorings for code smells yourself, you’ll quickly see that they often end up in the same ballpark. A lot of refactorings are simply different strategies to get to a point where you have classes that are concise, well organized, and focused on a small amount of responsibilities. I think it’s fair to say that if you can achieve that, you’ll be ahead of the pack most of the time—not that being ahead of others is so important, but such class design is simply often missing in code from people before they’re considered “experts”.

So why not get into the game early and build a concrete foundation for designing your code. Don’t believe what may be your own narrative that this is an advanced topic that you should put off for a while until you’re ready. Even if you’re a newbie, if you take small steps you can wrap your head around smells and their refactorings a lot earlier than you might think.

Before we dive into the mechanics, I want to repeat an important point from the first article. Not every smell is inherently bad, and not every refactoring is always worth it. You have to decide on the spot—when you have all the relevant information at your disposal—if your code is more stable after a refactoring and if it’s worth your time to fix the smell.

Feature Envy

Let’s revisit an example from the previous article. We extracted a long list of parameters for #assign_new_mission into a parameter object via the Mission class. So far so cool.

M with feature envy

class M
  def assign_new_mission(mission)
    print "Mission #{mission.mission_name} has been assigned to #{mission.agent_name} with the objective to #{mission.objective}."
    if mission.licence_to_kill
      print " The licence to kill has been granted."
    else
      print " The licence to kill has not been granted."
    end
  end
end

class Mission
  attr_reader :mission_name, :agent_name, :objective, :licence_to_kill

  def initialize(mission_name: mission_name, agent_name: agent_name, objective: objective, licence_to_kill: licence_to_kill)
    @mission_name    = mission_name
    @agent_name      = agent_name
    @objective       = objective
    @licence_to_kill = licence_to_kill
  end
end

m = M.new

mission = Mission.new(mission_name: 'Octopussy', agent_name: 'James Bond', objective: 'find the nuclear device', licence_to_kill: true)

m.assign_new_mission(mission)

# => "Mission Octopussy has been assigned to James Bond with the objective to find the nuclear device. The licence to kill has been granted."

I briefly mentioned how we can simplify the M class even more by moving the method #assign_new_mission to the new class for the parameter object. What I didn’t address was the fact that M had an easily curable form of feature envy as well. M was way too nosy about attributes of Mission. Put differently, she asked way too many “questions” about the mission object. It’s not only a bad case of micromanagement but also a very common code smell.

Let me show you what I mean. In M#assign_new_mission, M is “envious” about the data in the new parameter object and wants to access it all over the place.

• mission.mission_name
• mission.agent_name
• mission.objective
• mission.licence_to_kill

In addition to that, you also have a parameter object Mission that is only responsible for data right now—which is another smell, a Data Class.

This whole situation tells you basically that #assign_new_mission wants to be somewhere else and M doesn’t need to know the details of how missions get assigned. After all, why wouldn’t it be a mission’s responsibility to assign new missions? Remember to always to put things together that also change together.

M without feature envy

class M
  def assign_new_mission(mission)
    mission.assign
  end
end

class Mission
  attr_reader :mission_name, :agent_name, :objective, :licence_to_kill

  def initialize(mission_name: mission_name, agent_name: agent_name, objective: objective, licence_to_kill: licence_to_kill)
    @mission_name    = mission_name
    @agent_name      = agent_name
    @objective       = objective
    @licence_to_kill = licence_to_kill
  end

  def assign
    print "Mission #{mission_name} has been assigned to #{agent_name} with the objective to #{objective}."
    if licence_to_kill
      print " The licence to kill has been granted."
    else
      print " The licence to kill has not been granted."
    end
  end
end

m = M.new
mission = Mission.new(mission_name: 'Octopussy', agent_name: 'James Bond', objective: 'find the nuclear device', licence_to_kill: true)
m.assign_new_mission(mission)

As you can see, we simplified things quite a bit. The method slimmed down significantly and delegates the behaviour to the object in charge. M does not request mission data specifics anymore and certainly stays away from getting involved in how assignments get printed. Now she can focus on her real job and doesn’t need to be disturbed if any details of mission assignments change. More time for mind games and hunting down rogue agents. Win-win!

Feature envy breeds entanglement—by that I don’t mean the good kind, the one that lets information travel faster than light spookily—I’m talking about the one that over time might let your development momentum grind to an every more approaching halt. Not good! Why so? Ripple effects throughout your code will create resistance! A change in one place butterflies through all kinds of stuff and you end up as a kite in a hurricane. (Ok, a bit overly dramatic, but I give myself a B+ for the Bond reference in there.)

As a general antidote for feature envy, you want to aim for designing classes that are concerned mostly about their own stuff and have—if possible—single responsibilities. In short, classes should be something like friendly otakus. Socially that might not be the healthiest of behaviours, but for designing classes it often is a reasonable guideline to keep your momentum where it should be—moving forward!

Shotgun Surgery

The name is a little bit silly, isn’t? But at the same time it’s a pretty accurate description. Sounds like serious business, and it is! Luckily it’s not that hard to grasp, but nonetheless it’s one of the nastier code smells. Why? Because it breeds duplication like no other, and it’s easy to lose sight of all the changes you’d need to make to fix things. What happens during shotgun surgery is you make a change in one class/file and you also need to touch many other classes/files that need to be updated. Hope that doesn’t sound like a good time you’re in for.

For example, you may think you can get away with one little change in one place, and then realize that you have to wade through a whole bunch of files to make either the same change or fix something else that is broken because of it. Not good, not at all! That sounds more like a good reason why people start to hate the code they’re dealing with.

If you have a spectrum with DRY code on one side, then code that often needs shotgun surgery is pretty much on the opposite end. Don’t be lazy and let yourself enter that territory. I’m sure you’d rather open one file and apply your changes there and be done with it. That’s the kind of lazy you should strive for!

To avoid this smell, here’s a short list of symptoms you can look out for:

• Feature Envy
• Tight coupling
• Long Parameter List
• Any form of code duplication

What do we mean when we talk about code that is coupled? Let’s say we have objects A and B. If they are not coupled then you can change one of them without affecting the other. Otherwise you’ll more often than not also have to deal with the other object as well.

This is a problem, and shotgun surgery is a symptom for tight coupling as well. So always watch out for how easily you can change your code. If it’s relatively easy it means that your level of coupling is acceptably low. Having said that, I realize that your expectations would be unrealistic if you expect to be able to avoid coupling all the time at all costs. That’s not going to happen! You will find good reasons to decide against that urge—like replacing conditionals with Polymorphism. In such a case, a little bit of coupling, shotgun surgery and keeping the API of objects in sync is well worth getting rid of a ton of case statements via a Null Object (more on that in a later piece).

Most commonly you can apply one of the following refactorings to heal the wounds:

• Move Field
• Inline Class
• Extract Class
• Move Method

Let’s look at some code. This example is a slice of how a Spectre app handles payments between their contractors and evil clients. I simplified the payments a bit by having standard fees for both contractors and clients. So it doesn’t matter whether Spectre is tasked to kidnap a cat or extort a whole country: the fee stays the same. The same goes for what they pay their contractors. In the rare case an operation goes south and another Nr. 2 has to literally jump the shark, Spectre offers a full refund to keep evil clients happy. Spectre uses some proprietary payment gem that is basically a placeholder for any kind of payment processor.

In the first example below, it would be a pain if Spectre decided to use another library to handle payments. There would be more moving parts involved, but for demonstrating shotgun surgery this amount of complexity will do I think:

Example with shotgun surgery smell:

class EvilClient
  #...

  STANDARD_CHARGE = 10000000
  BONUS_CHARGE    = 10000000

  def accept_new_client
    PaymentGem.create_client(email)
  end

  def charge_for_initializing_operation
    evil_client_id = PaymentGem.find_client(email).payments_id
    PaymentGem.charge(evil_client_id, STANDARD_CHARGE)
  end

  def charge_for_successful_operation
    evil_client_id = PaymentGem.find_client(email).payments_id
    PaymentGem.charge(evil_client_id, BONUS_CHARGE)
  end
end

class Operation
  #...

  REFUND_AMOUNT = 10000000

  def refund
    transaction_id = PaymentGem.find_transaction(payments_id)
    PaymentGem.refund(transaction_id, REFUND_AMOUNT)
  end
end 

class Contractor
  #...

  STANDARD_PAYOUT = 200000
  BONUS_PAYOUT    = 1000000

  def process_payout
    spectre_agent_id = PaymentGem.find_contractor(email).payments_id
    if operation.enemy_agent == 'James Bond' && operation.enemy_agent_status == 'Killed in action'
      PaymentGem.transfer_funds(spectre_agent_id, BONUS_PAYOUT)
    else
      PaymentGem.transfer_funds(spectre_agent_id, STANDARD_PAYOUT)
    end
  end
end

When you look at this code you should ask yourself: “Should the EvilClients class be really concerned about how the payment processor accepts new evil clients and how they are charged for operations?” Of course not! Is it a good idea to spread the various amounts to pay all over the place? Should the implementation details of the payments processor be showing up in any of these classes? Most definitely not!

Look at it from that way. If you want to change something about the way you handle payments, why would you need to open the EvilClient class? In other cases it could be a user or customer. If you think about it, it doesn’t make any sense to familiarize them with this process.

In this example, it should be easy to see that changes to the way you accept and transfer payments create ripple effects throughout your code. Also, if you wanted to change the amount you charge or transfer to your contractors, you’d need additional changes all over the place. Prime examples of shotgun surgery. And in this case we’re only dealing with three classes. Imagine your pain if a bit more realistic complexity is involved. Yep, that’s the stuff nightmares are made of. Let’s look at an example that is a bit more sane:

Example without shotgun surgery smell and extracted class:

class PaymentHandler
  STANDARD_CHARGE            = 10000000
  BONUS_CHARGE               = 10000000
  REFUND_AMOUNT              = 10000000
  STANDARD_CONTRACTOR_PAYOUT = 200000
  BONUS_CONTRACTOR_PAYOUT    = 1000000

  def initialize(payment_handler = PaymentGem)
    @payment_handler = payment_handler
  end

  def accept_new_client(evil_client)
    @payment_handler.create_client(evil_client.email)
  end

  def charge_for_initializing_operation(evil_client)
    evil_client_id = @payment_handler.find_client(evil_client.email).payments_id
    @payment_handler.charge(evil_client_id, STANDARD_CHARGE)
  end

  def charge_for_successful_operation(evil_client)
    evil_client_id = @payment_handler.find_client(evil_client.email).payments_id
    @payment_handler.charge(evil_client_id, BONUS_CHARGE)
  end

  def refund(operation)
    transaction_id = @payment_handler.find_transaction(operation.payments_id)
    @payment_handler.refund(transaction_id, REFUND_AMOUNT)
  end

  def contractor_payout(contractor)
    spectre_agent_id = @payment_handler.find_contractor(contractor.email).payments_id
    if operation.enemy_agent == 'James Bond' && operation.enemy_agent_status == 'Killed in action'
      @payment_handler.transfer_funds(spectre_agent_id, BONUS_CONTRACTOR_PAYOUT)
    else
      @payment_handler.transfer_funds(spectre_agent_id, STANDARD_CONTRACTOR_PAYOUT)
    end
  end
end

class EvilClient
  #...

  def accept_new_client
    PaymentHandler.new.accept_new_client(self)
  end

  def charge_for_initializing_operation
    PaymentHandler.new.charge_for_initializing_operation(self)
  end

  def charge_for_successful_operation(operation)
    PaymentHandler.new.charge_for_successful_operation(self)
  end
end

class Operation
  #...

  def refund
    PaymentHandler.new.refund(self)
  end
end

class Contractor
  #...

  def process_payout
    PaymentHandler.new.contractor_payout(self)
  end
end

What we did here is wrap the PaymentGem API in our own class. Now we have one central place where we apply our changes if we decide that, for example, a SpectrePaymentGem would work better for us. No more touching of multiple—to payments’ internals unrelated—files if we need to adapt to changes. In the classes that deal with payments, we simply instantiated the PaymentHandler and delegate the needed functionality. Easy, stable, and no reason to change.

And not only have we contained everything in a single file. Within the PaymentsHandler class, there is only one place we need to swap out and reference a possible new payment processor—in initialize. That is rad in my book. Sure, if the new payment service has a completely different API, you need to tweak the bodies of a couple of methods in PaymentHandler. It’s a tiny price to pay compared to full-on shotgun surgery—that’s more like surgery for a small splinter in your finger. Good deal!

If you’re not careful when you write tests for a payment processor like this—or any external service you need to rely on—you are possibly in for serious headaches when they change their API. They “suffer from change” as well, of course. And the question is not will they change their API, only when.

Through our encapsulation we’re in a much better position to stub our methods for the payment processor. Why? Because the methods we stub are our own and they only change when we want them to. That is a big win. If you’re new to testing and this is not completely clear to you, don’t worry about it. Take your time; this topic can be tricky at first. Because it’s such an advantage, I just wanted to mention it for the sake of completeness.

As you can see, I simplified payment processing quite a bit in this silly example. I could have cleaned the final result some more as well, but the point was to clearly demonstrate the smell and how you can get rid of it through abstraction.

If you’re not completely happy with this class and see opportunities for refactoring, I salute you—and am happy to take credit for it. I recommend you knock yourself out! A good start might be dealing with the way you find payments_ids. The class itself also got a bit crowded already…

Divergent Change

Divergent change is, in a way, the opposite of shotgun surgery—where you want to change one thing and need to blast that change through a bunch of different files. Here a single class is often changed for different reasons and in different ways. My recommendation is to identify parts that change together and extract them in a separate class that can focus on that single responsibility. These classes in turn should also have no more than one reason to change—if not, another divergent change smell is most likely waiting to bite you.

Classes that suffer from divergent change are ones that get changed a lot. With tools like Churn, you can measure how often particular parts of your code needed to change in the past. The more points you find on a class, the higher the probability that divergent change might be at work. I also wouldn’t be surprised if exactly these classes are the ones that cause the most bugs overall.

Don’t get me wrong: getting changed often is not directly the smell. It’s a useful symptom, though. Another very common and more explicit symptom is that this object needs to juggle more than one responsibility. The single responsibility principle SRP is an excellent guideline to prevent this code smell and to write more stable code in general. It can be tricky to follow, but nevertheless still worth the grind.

Let’s look at this nasty example below. I modified the shotgun surgery example a bit. Blofeld, head of Spectre, might be known to micromanage stuff, but I doubt he would be interested in half the stuff this class is involved with.

class Spectre

  STANDARD_CHARGE = 10000000
  STANDARD_PAYOUT = 200000

  def charge_for_initializing_operation(client)
    evil_client_id = PaymentGem.find_client(client.email).payments_id
    PaymentGem.charge(evil_client_id, STANDARD_CHARGE)
  end

  def contractor_payout(contractor)
    spectre_agent_id = PaymentGem.find_contractor(contractor.email).payments_id
    PaymentGem.transfer_funds(spectre_agent_id, STANDARD_PAYOUT)
  end

  def assign_new_operation(operation)
    operation.contractor = 'Some evil dude'
    operation.objective  = 'Steal a boatload of valuable stuff'
    operation.deadline   = 'Midnight, November 18th'
  end

  def print_operation_assignment(operation)
    print "#{operation.contractor} is assigned to #{operation.objective}. The mission deadline ends at #{operation.deadline}."
  end

  def dispose_of_agent(spectre_agent)
    puts "You disappointed this organisation. You know how Spectre handles failure. Good bye #{spectre_agent.code_name}!"
  end
end

The Spectre class has way too many different things it is concerned about:

• Assigning new operations
• Charging for their dirty work
• Printing mission assignments
• Killing unsuccessful spectre agents
• Dealing with the PaymentGem internals
• Paying their Spectre agents/contractors
• It also knows about the amounts of money for charging and payout

Seven different responsibilities on a single class. Not good! You need to change how agents are disposed of? One vector for changing the Spectre class. You want to handle payouts differently? Another vector. You get the drift.

Although this example is far from being realistic, it still tells the story of how easy it is to unnecessarily amass behaviour that needs to change frequently in a single place. We can do better!

class Spectre
  # ...

  def dispose_of_agent(spectre_agent)
    puts "You disappointed this organisation. You know how Spectre handles failure. Good bye #{spectre_agent.code_name}!"
  end
end

class PaymentHandler
  STANDARD_CHARGE            = 10000000
  STANDARD_CONTRACTOR_PAYOUT = 200000

  #...

  def initialize(payment_handler = PaymentGem)
    @payment_handler = payment_handler
  end

  def charge_for_initializing_operation(evil_client)
    evil_client_id = @payment_handler.find_client(evil_client.email).payments_id
    @payment_handler.charge(evil_client_id, STANDARD_CHARGE)
  end

  def contractor_payout(contractor)
    spectre_agent_id = @payment_handler.find_contractor(contractor.email).payments_id
    @payment_handler.transfer_funds(spectre_agent_id, STANDARD_CONTRACTOR_PAYOUT)
    end
  end
end

class EvilClient
  #...

  def charge_for_initializing_operation
    PaymentHandler.new.charge_for_initializing_operation(self)
  end
end

class Contractor
  #...

  def process_payout
    PaymentHandler.new.contractor_payout(self)
  end
end

class Operation
  attr_accessor :contractor, :objective, :deadline

  def initialize(attrs = {})
    @contractor = attrs[:contractor] 
    @objective  = attrs[:objective] 
    @deadline   = attrs[:deadline] 
  end

  def print_operation_assignment
    print "#{contractor} is assigned to #{objective}. The mission deadline ends at #{deadline}."
  end
end

Here we extracted a bunch of classes and gave them their own unique responsibilities—and therefore their own contained reason to change.

You want to handle payments differently? Now you won’t need to touch the Spectre class. You need to charge or pay out differently? Again, no need to open the file for Spectre. Printing operation assignments is now the business of operation—where it belongs. That’s it. Not too complicated, I think, but definitely one of the more common smells you should learn to handle early.

Final Thoughts

I hope you got to the point where you feel ready to try these refactorings in your own code and have an easier time identifying code smells around you. Beware that we just got started, but that you already tackled a couple of big ones. I bet it was not as tricky as you once might have thought!

Sure, real-world examples will be a lot more challenging, but if you have understood the mechanics and patterns to spot smells, you’ll surely be able to adapt quickly to realistic complexities.