In Progress
Unit 1, Lesson 21
In Progress

Assisted Refactoring

Video transcript & code

It's another refactoring episode, I hope you don't mind! I'm doing things a little differently today, see if you can figure out what has changed…

Here are three methods which look suspiciously similar to each other. One is for fetching a list of all episodes, one for fetching only free episodes, and one for fetching a list of episodes sorted by episode number.

Each method has the same basic pattern: first, get a list of episode rows from the database. Then, get a list of episodes from a "remote mapper" object. Then go over the list returned from the remote mapper. For any episodes which have associated data in the database rows, augment the episode object using the #load_model method.

class EpisodeDataMapper
  # ...
  def all
    episode_rows = table.all
    remote_mapper.all.map{|episode|
      #noinspection RubyAssignmentExpressionInConditionalInspection
      if row = episode_rows.detect { |r| r[:dpd_id] == episode.id }
        load_model(episode, row)
      end
      episode
    }
  end

  def free_episodes
    episode_rows = table.all
    remote_mapper.free_episodes.map{ |episode|
      #noinspection RubyAssignmentExpressionInConditionalInspection
      if row = episode_rows.detect { |r| r[:dpd_id] == episode.id }
        load_model(episode, row)
      end
      episode
    }
  end

  def by_number
    episode_rows = table.all
    remote_mapper.by_number.map{ |episode|
      #noinspection RubyAssignmentExpressionInConditionalInspection
      if row = episode_rows.detect { |r| r[:dpd_id] == episode.id }
        load_model(episode, row)
      end
      episode
    }
  end
  # ...
end

Let's see if we can factor out the commonality in these methods.

First of all, we can see that the episode loading code is always the same. We perform an extract method refactoring to pull out a method called #fill_in_episode.

What's this? It says that the same code has been discovered in two other locations in the file! Why yes, I think we would like to replace those instances as well.

Now that we've extracted this method out, we move it down into the private section.

class EpisodeDataMapper
  # ...
  def all
    episode_rows = table.all
    remote_mapper.all.map{|episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  def free_episodes
    episode_rows = table.all
    remote_mapper.free_episodes.map{ |episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  def by_number
    episode_rows = table.all
    remote_mapper.by_number.map{ |episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  private

  def fill_in_episode(episode, episode_rows)
    #noinspection RubyAssignmentExpressionInConditionalInspection
    if row = episode_rows.detect { |r| r[:dpd_id] == episode.id }
      load_model(episode, row)
    end
  end
  # ...
end

Each of these three methods differs in how it selects episodes from the remote_mapper. Before we go any further, let's isolate that point of variance a little better. We'll do that by performing extract variable refactorings in each method. Each time, we call our extracted variable episode_list.

What we're doing now is effectively "defactoring". We're adding code, rather than reducing it. This may seem counterintuitive, but it's an essential part of refactoring. It's often very difficult to see where the "seams" are in a piece of code without first extracting some variables to name the different parts that go into it.

class EpisodeDataMapper
  # ...
  def all
    episode_rows = table.all
    episode_list = remote_mapper.all
    episode_list.map{|episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  def free_episodes
    episode_rows = table.all
    episode_list = remote_mapper.free_episodes
    episode_list.map{ |episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  def by_number
    episode_rows = table.all
    episode_list = remote_mapper.by_number
    episode_list.map{ |episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  private

  def fill_in_episode(episode, episode_rows)
    #noinspection RubyAssignmentExpressionInConditionalInspection
    if row = episode_rows.detect { |r| r[:dpd_id] == episode.id }
      load_model(episode, row)
    end
  end
  # ...
end

Now we go over each method again. In each one we move the extracted variable up to the top of the method.

class EpisodeDataMapper
  # ...
  def all
    episode_list = remote_mapper.all
    episode_rows = table.all
    episode_list.map{|episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  def free_episodes
    episode_list = remote_mapper.free_episodes
    episode_rows = table.all
    episode_list.map{ |episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  def by_number
    episode_list = remote_mapper.by_number
    episode_rows = table.all
    episode_list.map{ |episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  private

  def fill_in_episode(episode, episode_rows)
    #noinspection RubyAssignmentExpressionInConditionalInspection
    if row = episode_rows.detect { |r| r[:dpd_id] == episode.id }
      load_model(episode, row)
    end
  end
  # ...
end

Why do we perform this step? Well, take a look at how this method is now laid out. We now have two sections: at the top of the method is the part that differs from other methods. At the bottom is the code which is shared in common with all the other methods. By pulling out these variables and then moving them up, we've fully pulled apart the parts that change from the parts that are the same.

Which means we can now easily perform another "extract method" refactoring on the common parts. Once again, we allow the editor to replace each occurrence of the shared code. And then once again we move the extracted method to the private section of the class.

class EpisodeDataMapper
  # ...
  def all
    episode_list = remote_mapper.all
    fill_in_episode_list(episode_list)
  end

  def free_episodes
    episode_list = remote_mapper.free_episodes
    fill_in_episode_list(episode_list)
  end

  def by_number
    episode_list = remote_mapper.by_number
    fill_in_episode_list(episode_list)
  end

  private

  def fill_in_episode_list(episode_list)
    episode_rows = table.all
    episode_list.map{ |episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  def fill_in_episode(episode, episode_rows)
    #noinspection RubyAssignmentExpressionInConditionalInspection
    if row = episode_rows.detect { |r| r[:dpd_id] == episode.id }
      load_model(episode, row)
    end
  end
  # ...
end

Now that we've extracted this second method, the episode_list variables seem extraneous. We go over each one and perform the "inline variable" refactoring.

class EpisodeDataMapper
  # ...
  def all
    fill_in_episode_list(remote_mapper.all)
  end

  def free_episodes
    fill_in_episode_list(remote_mapper.free_episodes)
  end

  def by_number
    fill_in_episode_list(remote_mapper.by_number)
  end

  private

  def fill_in_episode_list(episode_list)
    episode_rows = table.all
    episode_list.map{ |episode|
      fill_in_episode(episode, episode_rows)
      episode
    }
  end

  def fill_in_episode(episode, episode_rows)
    #noinspection RubyAssignmentExpressionInConditionalInspection
    if row = episode_rows.detect { |r| r[:dpd_id] == episode.id }
      load_model(episode, row)
    end
  end
  # ...
end

And there we have it: we've extracted all of the commonality from these three methods, using a sequence of mechanical refactorings. I want to draw special attention to the sequence of: extract variable, extract method, inline variable. I've found this sequence of refactorings to be a particularly powerful way to get a handle on the commonalities in a piece of code.

Now, you probably noticed something different about this episode. Normally I write code in Emacs. But today I used RubyMine.

I'm a huge fan of Emacs, and of text editors in general. But Emacs simply doesn't have the kind of advanced refactoring support that RubyMine offers. If you're used to coding Ruby with a text editor, it may come as a surprise just how easy some of these basic refactorings can be in an editor that can semantically analyze code.

I find that I code a little differently when I'm using RubyMine. There are changes that I might be more reluctant to make in Emacs, because I can't simply press a button to inline a variable (for example). I feel less resistance to trying big changes to how my code is organized when I'm using a refactoring IDE like RubyMine.

It's all too easy to fall into certain coding habits not because they are ideal, but because they are what our tools support. For this reason I like to vary my tools from time to time. That way I can discover which of my habits are based on good ideas, and which are just based on the limitations of my usual tools. Switching to RubyMine reminds me to refactor mercilessly, and I think that's a very good thing.

Happy hacking!

Responses