Topics

• Heads up
• Resistance
• Large Class / God Class
• Extract Class
• Long Method
• Long Parameter List

Heads Up

The following short series of articles is meant for slightly experienced Ruby developers and starters alike. I had the impression that code smells and their refactorings can be very daunting and intimidating to newbies—especially if they are not in the fortunate position to have mentors who can turn mystical programming concepts into shining light bulbs.

Having obviously walked in these shoes myself, I remembered that it felt unnecessarily foggy to get into code smells and refactorings.

On the one hand, authors expect a certain level of proficiency and therefore might not feel super compelled to provide the reader with the same amount of context as a newbie might need to comfortably dive into this world sooner.

As a consequence, maybe, newbies on the other hand form the impression that they should wait a bit longer until they are more advanced to learn about smells and refactorings. I do not agree with that approach and think that making this topic more approachable will help them design better software earlier in their career. At least I hope it helps to provide junior peeps with a solid head start.

So what are we talking about exactly when people mention code smells? Is it always a problem in your code? Not necessarily! Can you avoid them completely? I don’t think so! Do you mean code smells lead to broken code? Well, sometimes and sometimes not. Should it be my priority to fix them right away? Same answer, I fear: sometimes yes and sometimes you certainly should fry bigger fish first. Are you insane? Fair question at this point!

Before you continue diving into this whole smelly business, remember to take away one thing from all of this: Don’t try to fix every smell you encounter—this is most certainly a waste of your time!

It seems to me that code smells are a bit hard to wrap up in a nicely labeled box. There are all kinds of smells with various different options to address them. Also, different programming languages and frameworks are prone to different kinds of smells—but there are definitely a lot of common “genetic” strains among them. My attempt to describe code smells is to compare them with medical symptoms that tell you that you might have a problem. They can point to all sorts of latent problems and have a wide variety of solutions if diagnosed.

Thankfully they’re overall not as complicated as dealing with the human body—and psyche of course. It’s a fair comparison, though, because some of these symptoms need to be treated right away, and some others give you ample time to come up with a solution that is best for the “patient’s” overall well-being. If you have working code and you run into something smelly, you’ll have to make the hard decision if it’s worth the time to find a fix and if that refactoring improves the stability of your app.

That being said, if you stumble upon code which you can improve right away, it’s good advice to leave the code behind a bit better than before—even a tiny bit better adds up substantially over time.

Resistance

The quality of your code becomes questionable if the inclusion of new code becomes harder—like deciding where to put new code is a pain or comes with a lot of ripple effects throughout your codebase, for example. This is called resistance.

As a guideline for code quality, you can measure it always by how easy it is to introduce changes. If that is getting harder and harder, it’s definitely time to refactor and to take the last part of red-green-REFACTOR more seriously in the future.

Large Class / God Class

Let’s start with something fancy sounding—“God classes”—because I think they are particularly easy to grasp for beginners. God classes are a special case of a code smell called Large Class. In this section I’ll address both of them. If you have spent a little bit of time in Rails land, you probably have seen them so often that they look normal to you.

You surely remember the “fat models, skinny controller” mantra? Well actually, skinny is good for all these classes, but as a guideline it’s good advice for newbies I suppose.

God classes are objects that attract all sorts of knowledge and behaviour like a black hole. Your usual suspects most often include the User model and whatever problem (hopefully!) your app is trying to solve—first and foremost at least. A todo app might bulk up on the Todos model, a shopping app on Products, a photo app on Photos—you get the drift.

People call them god classes because they know too much. They have too many connections with other classes—mostly because someone was modeling them lazily. It is hard work, though, to keep god classes in check. They make it really easy to dump more responsibilities onto them, and as lots of Greek heroes would attest, it takes a bit of skill to divide and conquer “gods”.

The problem with them is that they become harder and harder to understand, especially for new team members, harder to change, and reusing them becomes less and less of an option the more gravity they’ve amassed. Oh yeah, you’re right, your tests are unnecessarily harder to write as well. In short, there is not really an upside to having large classes, and god classes in particular.

There are a couple of common symptoms/signs that your class needs some heroism/surgery:

• You need to scroll!
• Tons of private methods?
• Does your class have seven or more methods on it?
• Hard to tell what your class really does—concisely!
• Does your class have many reasons to change when your code evolves?

Also, if you squint at your class and think “Eh? Ew!” you might be on to something too. If all that sounds familiar, chances are good that you found yourself a fine specimen.

class CastingInviter
  EMAIL_REGEX = /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/

  attr_reader :message, :invitees, :casting

  def initialize(attributes = {})
	  @message  = attributes[:message]  || ''
	  @invitees = attributes[:invitees] || ''
	  @sender   = attributes[:sender]
	  @casting  = attributes[:casting]
	end

	def valid?
	  valid_message? && valid_invitees?
	end

	def deliver
    if valid?
      invitee_list.each do |email|
        invitation = create_invitation(email)
        Mailer.invitation_notification(invitation, @message)
      end
	  else
	    failure_message  = "Your #{ @casting } message couldn’t be sent. Invitees emails or message are invalid"
	    invitation = create_invitation(@sender)
      Mailer.invitation_notification(invitation, failure_message )
	  end
	end

	private

	def invalid_invitees
	  @invalid_invitees ||= invitee_list.map do |item|
      unless item.match(EMAIL_REGEX)
	      item
	    end
	  end.compact
	end

	def invitee_list
	  @invitee_list ||= @invitees.gsub(/\s+/, '').split(/[\n,;]+/)
	end

	def valid_message?
	  @message.present?
	end

	def valid_invitees?
	  invalid_invitees.empty?
	end

  def create_invitation(email)
    Invitation.create(
      casting:       @casting,
      sender:        @sender,
      invitee_email: email,
      status:        'pending'
    )
  end
end

Ugly fella, huh? Can you see how much nastiness is bundled in here? Of course I put a little cherry on top, but you will run into code like this sooner or later. Let’s think about what responsibilities this CastingInviter class has to juggle.

• Delivering email
• Checking for valid messages and email addresses
• Getting rid of white space
• Splitting email addresses on commas and semicolons

Should all of this be dumped on a class which just wants to deliver a casting call via deliver? Certainly not! If your method of invitation changes you can expect to run into some shotgun surgery. CastingInviter doesn’t need to know most of these details. That’s more the responsibility of some class that is specialized in dealing with email-related stuff. In the future, you’ll find many reasons to change your code in here as well.

Extract Class

So how should we deal with this? Often, extracting a class is a handy refactoring pattern that will present itself as a reasonable solution to such problems as big, convoluted classes—especially when the class in question deals with multiple responsibilities.

Private methods are often good candidates to start with—and easy marks as well. Sometimes you’ll need to extract even more than one class from such a bad boy—just don’t do it all in one step. Once you find enough coherent meat that seems to belong in a specialized object of its own that you can extract that functionality into a new class.

You create a new class and gradually move the functionality over—one by one. Move each method separately, and rename them if you see a reason to. Then reference the new class in the original one and delegate the needed functionality. Good thing you have test coverage (hopefully!) that lets you check if things still work properly every step of the way. Aim at being able to reuse your extracted classes as well. It’s easier to see how it’s done in action, so let’s read some code:

class CastingInviter

  attr_reader :message, :invitees, :casting

  def initialize(attributes = {})
    @message  = attributes[:message]  || ''
    @invitees = attributes[:invitees] || ''
    @casting  = attributes[:casting]
    @sender   = attributes[:sender]
  end
  
  def valid?
    casting_email_handler.valid?
  end
  
  def deliver
    casting_email_handler.deliver
  end
  
  private
  
  def casting_email_handler
    @casting_email_handler ||= CastingEmailHandler.new(
      message:  message, 
      invitees: invitees, 
      casting:  casting, 
      sender:   @sender
    )
  end
end

class CastingEmailHandler 
  EMAIL_REGEX = /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/

  def initialize(attr = {})
    @message  = attr[:message]  || ''
    @invitees = attr[:invitees] || ''
    @casting  = attr[:casting]
    @sender   = attr[:sender]
  end

  def valid?
    valid_message? && valid_invitees?
  end

  def deliver
    if valid?
      invitee_list.each do |email|
        invitation = create_invitation(email)
        Mailer.invitation_notification(invitation, @message)
      end
    else
      failure_message  = "Your #{ @casting } message couldn’t be sent. Invitees emails or message are invalid"
      invitation = create_invitation(@sender)
      Mailer.invitation_notification(invitation, failure_message )
    end
  end

  private

  def invalid_invitees
    @invalid_invitees ||= invitee_list.map do |item|
      unless item.match(EMAIL_REGEX)
        item
      end
    end.compact
  end

  def invitee_list
    @invitee_list ||= @invitees.gsub(/\s+/, '').split(/[\n,;]+/)
  end
  
  def valid_invitees?
    invalid_invitees.empty?
  end
  
  def valid_message?
    @message.present?
  end

  def create_invitation(email)
    Invitation.create(
      casting:       @casting,
      sender:        @sender,
      invitee_email: email,
      status:        'pending'
    )
  end
end

In this solution, you’ll not only see how this separation of concerns affects your code quality, it also reads a lot better and becomes easier to digest.

Here we delegate methods to a new class that is specialized in dealing with delivering these invitations via email. You have one dedicated place that checks if the messages and invitees are valid and how they need to be delivered. CastingInviter doesn’t need to know anything about these details, so we delegate these responsibilities to a new class CastingEmailHandler.

The knowledge of how to deliver and check for validity of these casting invitation emails is now all contained in our new extracted class. Do we have more code now? You bet! Was it worth it to separate concerns? Pretty sure! Can we go beyond that and refactor CastingEmailHandler some more? Absolutely! Knock yourself out!

In case you’re wondering about the valid? method on CastingEmailHandler and CastingInviter, this one is for RSpec to create a custom matcher. This lets me write something like:

expect(casting_inviter).to be_valid

Pretty handy, I think.

There are more techniques for dealing with large classes / god objects, and over the course of this series you’ll learn a couple of ways to refactor such objects.

There is no fixed prescription for dealing with these cases—it always depends, and it’s a case-by-case judgement call if you need to bring the big guns or if smaller, incremental refactoring techniques oblige best. I know, a bit frustrating at times. Following the Single Responsibility Principle (SRP) will go a long way, though, and is a good nose to follow.

Long Method

Having methods that got a little big is one of the most common things you encounter as a developer. In general, you want to know at a glance what a method is supposed to do. It should also have only one level of nesting or one level of abstraction. In short, avoid writing complicated methods.

I know this sounds hard, and it often is. A solution that comes up frequently is extracting parts of the method into one or more new functions. This refactoring technique is called the extract method—it’s one of the simplest but nonetheless very effective. As a nice side effect, your code becomes more readable if you name your methods appropriately.

Let’s take a look at feature specs where you’ll need this technique a lot. I remember getting introduced to the extract method while writing such feature specs and how amazing it felt when the lightbulb went on. Because feature specs like this are easy to understand, they are a good candidate for demonstration. Plus you’ll run into similar scenarios over and over when you write your specs.

spec/features/some_feature_spec.rb

require 'rails_helper'

feature 'M marks mission as complete' do
  scenario 'successfully' do
    visit_root_path
    fill_in      'Email', with: '[email protected]'
    click_button 'Submit'
    visit missions_path
    click_on     'Create Mission' 
    fill_in      'Mission Name', with: 'Project Moonraker'
    click_button 'Submit'

    within "li:contains('Project Moonraker')" do
      click_on 'Mission completed'
    end

    expect(page).to have_css 'ul.missions li.mission-name.completed', text: 'Project Moonraker'
  end
end

As you can easily see, there is a lot going on in this scenario. You go to the index page, sign in and create a mission for the setup, and then exercise via marking the mission as complete, and finally you verify the behaviour. No rocket science, but also not clean and definitely not composed for reusability. We can do better than that:

spec/features/some_feature_spec.rb

require 'rails_helper'

feature 'M marks mission as complete' do
  scenario 'successfully' do
    sign_in_as '[email protected]'
    create_classified_mission_named 'Project Moonraker'
  
    mark_mission_as_complete        'Project Moonraker'
  
    agent_sees_completed_mission    'Project Moonraker'
  end
end
  
def create_classified_mission_named(mission_name)
	visit missions_path
	click_on     'Create Mission' 
	fill_in      'Mission Name', with: mission_name
	click_button 'Submit'
	end

def mark_mission_as_complete(mission_name)
	within "li:contains('#{mission_name}')" do
	  click_on 'Mission completed'
	end
end

def agent_sees_completed_mission(mission_name)
	expect(page).to have_css 'ul.missions li.mission-name.completed', text: mission_name
end

def sign_in_as(email)
	visit root_path
	fill_in      'Email', with: email
	click_button 'Submit'
end

Here we extracted four methods that can be easily reused in other tests now. I hope it’s clear that we hit three birds with one stone. The feature is a lot more concise, it reads better, and it’s made up of extracted components without duplication.

Let’s imagine you’d written all kinds of similar scenarios without extracting these methods and you wanted to change some implementation. Now you wish you’d taken the time to refactor your tests and had one central place to apply your changes.

Sure, there is an even better way to deal with feature specs like this—Page Objects, for example—but that’s not our scope for today. I guess that’s all you need to know about extracting methods. You can apply this refactoring pattern everywhere in your code—not only in specs, of course. In terms of frequency of use, my guess is that it will be your number one technique to improve the quality of your code. Have fun!

Long Parameter List

Let’s close this article with an example of how you can slim down your parameters. It gets tedious pretty fast when you have to feed your methods more than one or two arguments. Wouldn’t it be nice to drop in one object instead? That’s exactly what you can do if you introduce a parameter object.

All these parameters are not only a pain to write and to keep in order, but can also lead to code duplication—and we certainly want to avoid that wherever possible. What I like especially about this refactoring technique is how this affects other methods inside as well. You often are able to get rid of a lot of parameter junk down in the food chain.

Let’s go over this simple example. M can assign a new mission and needs a mission name, an agent, and an objective. M is also able to switch agents’ double 0 status—meaning their licence to kill.

class M
  def assign_new_mission(mission_name, agent_name, objective, licence_to_kill: nil)
    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
m.assign_new_mission('Octopussy', 'James Bond', 'find the nuclear device', licence_to_kill: true)
# => Mission Octopussy has been assigned to James Bond with the objective to find the nuclear device. The licence to kill has been granted. 

When you look at this and ask what happens when the mission “parameters” grow in complexity, you’re already onto something. That’s a pain point that you can only solve if you pass in a single object that has all the information you need. More often than not, this also helps you to stay away from changing the method if the parameter object changes for some reason.

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

class M
  def assign_new_mission(mission)
	  mission.assign
	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.

So we created a new object, Mission, that is solely focused on providing M with the information needed to assign a new mission and provide #assign_new_mission with a singular parameter object. No need to pass in these pesky parameters yourself. Instead you tell the object to reveal the information you need inside the method itself. Additionally, we also extracted some behaviour—the information how to print—into the new Mission object.

Why should M need to know about how to print mission assignments? The new #assign also benefited from extraction by losing some weight because we didn’t need to pass in the parameter object—so no need to write stuff like mission.mission_name, mission.agent_name and so on. Now we just use our attr_reader(s), which is much cleaner than without the extraction. You dig?

What’s also handy about this is that Mission might collect all sorts of additional methods or states that are nicely encapsulated in one place and ready for you to access.

With this technique you’ll end up with methods that are more concise, tend to read better, and avoid repeating the same group of parameters all over the place. Pretty good deal! Getting rid of identical groups of parameters is also an important strategy for DRY code.

Try to look out for extracting more than just your data. If you can place behaviour in the new class as well, you’ll have objects that are more useful—otherwise they’ll quickly start to smell as well.

Sure, most of the time you’ll run into more complicated versions of that—and your tests will certainly also need to be adapted simultaneously during such refactorings—but if you have that simple example under your belt, you’ll be ready for action.

I’m gonna watch the new Bond now. Heard it’s not that good, though…

Update: Saw Spectre. My verdict: compared to Skyfall—which was MEH imho—Spectre was wawawiwa!