Recently I got to see a great example of when small methods and classes are good and when they’re not so good, thanks to a refactoring session and a fellow nerd’s code review. Let’s take a look at it together!

Where We Begin

Fellow Nerd Nathan was working on Big Nerd Ranch’s “bench report,” a report of how many developers are on the “bench” instead of on billable projects. This report shows us things like “in 3 weeks we’ll have 2 iOS developers and 1 Android developer available, unless we book some more work before then.” If a developer does multiple kinds of work, their availability is added under their “primary service”—what they mainly do.

Nathan was having trouble following the code because there were a lot of messages being sent back-and-forth between objects, and it wasn’t always obvious why. So he rewrote the code to be a single Ruby module with a few large methods, and it was a lot easier to understand. But he had a question: was the smallness of the methods and objects the root of the problem? Or was there something else going on?

I decided to try to answer this question by applying small refactorings to the code, paying attention to whether they moved the code back toward its former needless indirection. I wanted to see if there were some small methods and objects that improved readability, and others that hindered it.

Here’s the code I started with, after Nathan had simplified it to a single method. One particularly nice thing about this structure is that it helps you see the fact that it’s building up a nested hash:

  def self.calculate_service_availabilities(users, user_schedules, iteration_date_ranges)
    users.reduce({}) do |service_availabilities, user|
      primary_service = CurrentPrimaryServiceQuery.new(user).current_primary_service.try(:name)
      if primary_service.nil?
        service_availabilities
      else
        iteration_date_ranges.each do |iteration_date_range|
          user_availability = UserAvailability.new(
            user, user_schedule: user_schedules.fetch(user)
          ).availability_during(iteration_date_range)
          if user_availability >= MINIMUM_DISPLAYABLE_AVAILABILITY
            service_availabilities[primary_service] ||= {}
            service_availabilities[primary_service][iteration_date_range] ||= blank_avail_val
            service_availabilities[primary_service][iteration_date_range][:availabilities] << user_availability / 100.0
            service_availabilities[primary_service][iteration_date_range][:user_ids] << user.id
          end
        end
        service_availabilities
      end
    end
  end

Encapsulate a Block of Code

In this first post, we’ll focus on the Extract Method refactoring. In its most basic form, Extract Method involves taking a few lines of closely-related code, moving them into a method, then replacing those lines with a call to the new method. I did this with the user_availability calculation:

         iteration_date_ranges.each do |iteration_date_range|
-          user_availability = UserAvailability.new(
-            user, user_schedule: user_schedules.fetch(user)
-          ).availability_during(iteration_date_range)
+          user_availability = user_availability(user, user_schedules, iteration_date_range)
           if user_availability >= MINIMUM_DISPLAYABLE_AVAILABILITY

 ...

+  def self.user_availability(user, user_schedules, iteration_date_range)
+    UserAvailability.new(
+      user, user_schedule: user_schedules.fetch(user)
+    ).availability_during(iteration_date_range)
+  end

This is a helpful method extraction because the calculate_service_availabilities method doesn’t need to know how a user_availability is calculated—it just needs the data. By extracting a method, we’re able to just say what we want (the user availability) instead of the details of how it’s calculated.

Wrap an External Dependency

Another reason to extract a method is to isolate a dependency on another class or module. The previous method extraction did this, but there was another extraction I did to isolate a dependency even though it was only a single line of code:

     users.reduce({}) do |service_availabilities, user|
-      primary_service = CurrentPrimaryServiceQuery.new(user).current_primary_service.try(:name)
+      primary_service = primary_service_name(user)
       if primary_service.nil?

 ...

+  def self.primary_service_name(user)
+    CurrentPrimaryServiceQuery.new(user).current_primary_service.try(:name)
+  end

Even though it’s only one line, it’s a somewhat complex sequence of methods. I instantiate a CurrentPrimaryServiceQuery, then send it a message, then send another message to the result. (If this is setting off your Law of Demeter warning bells, we’ll leave that discussion for another time!) This method chain clutters up the calculate_service_availabilities method with low-level details, making it harder to quickly understand. As with the last extraction, we hide the details of how to retrieve this data and replace them with a method call that says what we are trying to accomplish.

Like before, we extract this line into a method and give it a name: primary_service_name(). Now the calculate_service_availabilities doesn’t need to know whether we’re interfacing with another class: that dependency is an implementation detail of the primary_service_name method.

Name a Concept

To take method extractions one step further, you can extract a single-line method even when you aren’t isolating an external dependency. Take a look at this example:

          user_availability = user_availability(user, user_schedules, iteration_date_range)
-         if user_availability >= MINIMUM_DISPLAYABLE_AVAILABILITY
+         unless insignificant?(user_availability)
            service_availabilities[primary_service] ||= {}

 ...

+  def self.insignificant?(user_availability)
+    user_availability < MINIMUM_DISPLAYABLE_AVAILABILITY
+  end

The reason to consider performing this extraction is to raise the level of abstraction. We’re stating that we’re checking if the availability is significant, rather than describing how we perform that check.

Before the extraction, you had to read the comparison to the constant and do a mental translation in your head to say “oh, this is checking if user_availability is too small to be significant.” That may seem like a small cost, but every long, difficult-to-understand method is made up of dozens of lines each with this cost. It adds up.

A Bad Extraction

I’ve shown why Nathan and I agreed each of the above method extractions add value. Now let’s look at some extractions I proposed that we decided aren’t a great idea.

It bothered me that calculate_service_availabilities was still pretty long, and it had several levels of nested loops and conditionals. So I split these out into two additional methods:

     users.reduce({}) do |service_availabilities, user|
       primary_service = primary_service_name(user)
       if primary_service.nil?
         service_availabilities
       else
-        iteration_date_ranges.each do |iteration_date_range|
-          user_availability = user_availability(user, user_schedules, iteration_date_range)
-          if displayable?(user_availability)
-            service_availabilities[primary_service] ||= {}
-            service_availabilities[primary_service][iteration_date_range] ||= blank_avail_val
-            service_availabilities[primary_service][iteration_date_range][:availabilities] << user_availability / 100.0
-            service_availabilities[primary_service][iteration_date_range][:user_ids] << user.id
-          end
-        end
+        service_availabilities[primary_service] ||= {}
+        add_availabilities_for_date_ranges(service_availabilities[primary_service], iteration_date_ranges, user, user_schedules)
         service_availabilities
       end
     end
   end

 ...

+  def self.add_availabilities_for_date_ranges(availability_for_service, iteration_date_ranges, user, user_schedules)
+    iteration_date_ranges.each do |iteration_date_range|
+      user_availability = user_availability(user, user_schedules, iteration_date_range)
+      add_availability_if_displayable(availability_for_service, iteration_date_range, user, user_availability)
+    end
+  end
+  
+  def self.add_availability_if_displayable(availability_for_service, iteration_date_range, user, user_availability)
+    if displayable?(user_availability)
+      availability_for_service[iteration_date_range] ||= blank_avail_val
+      availability_for_service[iteration_date_range][:availabilities] << user_availability / 100.0
+      availability_for_service[iteration_date_range][:user_ids] << user.id
+    end
+  end

This is pretty hard to follow! Here’s what’s going on:

1. Inside the reduce block where we loop through the users, we no longer loop through the iteration_date_ranges directly. Instead, we call out to an add_availabilities_for_date_ranges method that does the looping.
2. In that method, we don’t add the user’s availability to the result hash directly; we call out to an add_availability_if_displayable method to do it.

What Went Wrong?

So what’s wrong with these extractions? The way Nathan describes it, “I want to minimize context switches. Context switches should be few in number and short in distance.” Does this mean you should never extract methods? No, because understanding what a method does doesn’t always require a context switch. If you can tell what the method does just from reading the name, you don’t have to make a context switch to read the body.

To test this, take a look at the add_availabilities_for_date_ranges method in isolation:

  def self.add_availabilities_for_date_ranges(availability_for_service, iteration_date_ranges, user, user_schedules)
    iteration_date_ranges.each do |iteration_date_range|
      user_availability = user_availability(user, user_schedules, iteration_date_range)
      add_availability_if_displayable(availability_for_service, iteration_date_range, user, user_availability)
    end
  end

Let’s evaluate this method based on a question Nathan asked: “Do I have to go somewhere else to understand what I’m reading?” Yup. Here are a few examples:

• It’s not obvious what the availability_for_service parameter is without looking at the calling method.
• It’s not obvious what the call to add_availability_if_displayable accomplishes without reading that method.

These problems stem from the fact that add_availabilities_for_date_ranges doesn’t introduce a good abstraction; it just moves the details away from the place where you need to know about them. Since you have to read all the methods to understand any of them, it would be easier to keep them one big method.

But is there a different abstraction that would break up this big method well? There are a number of details related to the hash structure throughout the method: indexing multiple levels into it, setting up default values, deciding whether or not to add new data. Maybe these could be extracted into an object; we’ll come back to this in a future post. But for now, for us, there seems to be very little benefit to extracting these methods, and lots of cost.

Good Method Extractions

The goal behind extracting methods is increasing the readability and maintainability of your code. If you include all the details inline in a huge method then you force yourself to think through all the details every time you need to work with any of them. But it’s even worse if you extract necessary details, because you still need to think through all those details, and you also need to switch contexts to do it! The right approach is to find coherent chunks of code that can be given a good name: a method name that tells you everything you need to know about them. This is an important way to navigate between the two extremes of “overdesign” and “no design.”

Extract Method wasn’t the only refactoring I performed. In my next post we’ll look at a few different classes I extracted to see the pros and cons—and you’ll get to find out what happens when nerds… disagree! (gasp)

Until then, as you’re working on your code, keep an eye out for long methods and see if you can abstract away some of the details with the Extract Method refactoring. And on the flip side, if you find yourself switching context too often, see if a method has been extracted that may not be pulling its weight.