A practical guide to changing code so you can understand it

When I started working on more complex projects, I had a substantial amount of fear. I felt obligated to understand everything I was working on, top to bottom. Test Driven Development mitigates that fear. With Refactoring Tools in my tool belt, I wade into murky, untested code with joy. Here are some practical tips to help you start poking at that menu option in your IDE you’ve been ignoring.

The premise

You’re a new hire at a startup called OatmealRaise-in that helps bakeries maximize the number of raisin-based baked goods their customers will purchase. (I’m the sole investor, raisins are the best baking ingredient).

There seems to be a bug in our Ruby tool for reporting. It says no one is buying oatmeal raisin cookies. Strange! We manage to track down the report generator where we discover:

def calc_bpper_beep_jeep(object)
  bana = object.jeep
  object.bakery_products.group_by { |bp| bp.bakeable.baker.id }.map do |beep_jeep, bpper|
    if !fetch(bana)
      Alert::RaisinsNeeded.call
    else
      {
        beep_jeep => {
          baked_goods: bpper.select { |t| t.is_a? Oven::Baking::Bake::Recipe }.sum(0, &:count),
          samples: bpper.select { |t| t.is_a?(Oven::Baking::Bake::Sample) || t.is_a?(Oven::Baking::Bake::SampleReturned) }.sum(0, &:count),
          stale_product: bpper.select { |t| t.is_a? Oven::Baking::Bake::Stale }.sum(0, &:count)
        }
      }
    end
  end
end

Our keen eyes start staring at the shape of this code, its number of chained methods, and less-than-helpful variable names. Even worse, there are no tests.

We might be tempted to start top-down by wrapping the entire method with some characterization tests and working from the outside in to see what explodes. That’s still an approachable path given the size of this method, but we’d certainly have to do a lot of work to get enough context packed in our brain to understand. Likely that will include a lot of test setup, which can be a smell.

Instead, I’d like to offer you some tools to be more surgical in your approach to testing. Small steps to help us change the code so we can understand the code.

Refactoring tools to the rescue!

Whatever your favourite editing tool, you’ll likely have built-in Refactoring Tools. VSCode has them. So does JetBrains.

There’s a lot of content on how to use these tools, so I won’t bore you with that.

I’m interested in the “why” and the insight the tools help us generate.

There are a number of incredibly powerful tools, but we can get a lot of mileage by leaning heavily on the most straightforward ones:

• Introduce Variable
• Extract Method
• Rename

Extract method to the rescue

We’re most interested in why baked_goods seems to be showing $0 in sales; without changing any of the surrounding code, we can lean on our Extract Method.

Again, our whole method looks like this:

def calc_bpper_beep_jeep(object)
  bana = object.jeep
  object.bakery_products.group_by { |bp| bp.bakeable.baker.id }.map do |beep_jeep, bpper|
    if !fetch(bana)
      Alert::RasinsNeeded.call
    else
      {
        beep_jeep => {
          baked_goods: bpper.select { |t| t.is_a? Oven::Baking::Bake::Recipe }.sum(0, &:count),
          samples: bpper.select { |t| t.is_a?(Oven::Baking::Bake::Sample) || t.is_a?(Oven::Baking::Bake::SampleReturned) }.sum(0, &:count),
          stale_product: bpper.select { |t| t.is_a? Oven::Baking::Bake::Stale }.sum(0, &:count)
        }
      }
    end
  end
end

My eyes glaze over (sorry) when I see all the chained methods and variables I don’t understand. So, calling Extract Method on the baked_goods select can change this:

      {
        beep_jeep => {
          baked_goods: bpper.select { |t| t.is_a? Oven::Baking::Bake::Recipe }.sum(0, &:count),
          samples: bpper.select { |t| t.is_a?(Oven::Baking::Bake::Sample) || t.is_a?(Oven::Baking::Bake::SampleReturned) }.sum(0, &:count),
          stale_product: bpper.select { |t| t.is_a? Oven::Baking::Bake::Stale }.sum(0, &:count)
        }
      }

into this:

def calc_bpper_beep_jeep(object)
  # ... setup stuff
      {
        beep_jeep => {
          baked_goods: sum_baked_goods(bpper), # Moving this select call to a method
          samples: bpper.select { |t| t.is_a?(Oven::Baking::Bake::Sample) || t.is_a?(Oven::Baking::Bake::SampleReturned) }.sum(0, &:count),
          stale_product: bpper.select { |t| t.is_a? Oven::Baking::Bake::Stale }.sum(0, &:count)
        }
      }
end

def sum_baked_goods(bpper)
  bpper.select { |t| t.is_a? Oven::Baking::Bake::Recipe }.sum(0, &:count)
end

Extract Method automatically shoves our dependency in as a parameter to our new method. Now we can write tests for this more manageable piece of code. Smaller code means we can poke at what bpper actually is, and discover what edge cases are cropping up.

This could very well be enough to identify the change you need, resolve your bug, and move along.

We can choose to refactor sum_baked_goods further or remove duplication across all of our other select statements.

I like to tidy as I go. Our tests give us enough context to recognize bpper is a collection of baked products. That might help us make a new connection to the surrounding code: object.bakery_products.

Many of us are comfortable with the Rename refactoring, so let’s re-introduce some explicitness.

def calc_bpper_beep_jeep(object)
  bana = object.jeep
  object.bakery_products.group_by { |bp| bp.bakeable.baker.id }.map do |beep_jeep, baked_products| # more helpful variables!
    if !fetch(bana)
      Alert::RasinsNeeded.call
    else
      {
        beep_jeep => {
          baked_goods: sum_baked_goods(baked_products),
          samples: sum_samples(baked_products), # similar extract / test
          stale_product: sum_stale_product(baked_products)
        }
      }
    end
  end
end

def sum_baked_goods(baked_products)
  baked_products.select #... stuff
end

def sum_samples(baked_products)
  baked_products.select #... stuff
end

def sum_stale_product(baked_products)
  baked_products.select #... stuff
end

Even better, we notice that the method name has that same bpper designation. Applying Rename can help that method make more sense where it’s called. We can safely make sweeping changes to the footprint of this method across the codebase without fear of breaking anything.

calc_bpper_beep_jeep(object) -> calc_baked_products_per_beep_jeep(object)

With some simple refactoring, we’ve isolated our bug and made the code a bit more explicit for whoever comes next.

Guess what? New requirements!

Like all good software, we’re asked to change our code again. We’re deeper into the project and understand that the report lists all the products baked by each baker on our staff. We’d like to improve our product to show a detailed breakdown of items that have raisins like we promised our investor we would.

Happily enough, we’re already familiar with the report code:

def calc_baked_products_per_beep_jeep(object)
  bana = object.jeep

  object.bakery_products.group_by { |bp| bp.bakeable.baker.id }.map do |beep_jeep, baked_products|
    if !fetch(bana)
      Alert::RasinsNeeded.call
    else
      {
        beep_jeep => {
          baked_goods: sum_baked_goods(baked_products),
          samples: sum_samples(baked_products),
          stale_product: sum_stale_product(baked_products)
        }
      }
    end
  end
end

def sum_baked_goods(baked_products)
end
def sum_samples(baked_products)
end
def sum_stale_product(baked_products)
end

Introduce Variable weighs in

Suppose we want to move towards grouping by products with raisins instead of grouping by bakers. We can lean on the Introduce Variable tool to break up the chain of methods.

def calc_baked_products_per_beep_jeep(object)
  bana = object.jeep

  object.bakery_products.group_by { |bp| bp.bakeable.baker.id }.map do |beep_jeep, baked_products|

to

def calc_baked_products_per_beep_jeep(object)
  bana = object.jeep

  products_by_baker = object.bakery_products.group_by { |bp| bp.bakeable.baker.id }
  products_by_baker.map do |beep_jeep, baked_products|

So far, we’ve added indirection with no obvious gain. Trust the process.

Our next step is to call Extract Method on the bulk of our actual logic. I don’t know what to name it yet, so do_things can be a temporary placeholder.

def calc_baked_products_per_beep_jeep(object)
  bana = object.jeep

  products_by_baker = object.bakery_products.group_by { |bp| bp.bakeable.baker.id }
  do_things(bana, products_by_baker)
end

def do_things(bana, products_by_baker)
  products_by_baker.map do |beep_jeep, baked_products|
		# no changes in here! It's just wrapped in a new method
  end
end

Extracting do_things opens a seam to put in a different collection of baked products. It also reveals a dependency on this thing called bana. Bana sounds suspicious, like someone was putting Bananas in our baking instead of raisins. How dare they? Unfortunately, until I get a better grasp on what the bana dependency is, I’ll leave it alone.

With clarity around requirements clear, we can extend our test suite with our new raisin-based reporting and TDD our way to success. That extension might look like this:

def calc_baked_products_with_rasins(object)
  bana = object.jeep

  products_with_rasins = object.bakery_products.group_by { |bp| bp.bakeable.has_raisins? }
  do_things(bana, products_with_rasins)
end

Renaming for clarity

We’re extending our knowledge of what the original code was supposed to do by testing it in a smaller scope. Refactoring tools help us reveal where to do so.

We can be more explicit with some more renaming:

def calc_baked_products_per_beep_jeep(object)
  # setup
  baked_good_sales_by_category(bana, products_by_baker)
end

def calc_baked_products_with_rasins(object)
  # setup
  baked_good_sales_by_category(bana, products_with_rasins)
end

def baked_good_sales_by_category(bana, product_category)
  # Formerly called `do_things`
end

I tend to over-articulate my method names when context is missing. With further refactoring, those methods become more clear in their purpose, can be broken out further, tested better, and be given better names.

We’ve also revealed more obvious smells. bana is a strange name and is a dependency appearing in multiple methods. We can add a test harness and refactor away that dependency. Or, we might be at a “good enough” state to move on to more pressing challenges in our code.

How do we add raisins?

I think we can’t truly understand code until we get it wrapped in a test harness. Sometimes the thought of getting the code tested can be really overwhelming. If there are a lot of hidden dependencies, a whole batch of methods chained together, or unclear naming, it can be really challenging to find an entry point to get our fingers in.

By leaning on automated refactoring tools, we can shuffle code around with reasonable confidence that we’re not changing any behaviour. Often some light changes are all that we need to focus on and build that understanding through tests.

Tips

While Rename and Introduce Variable are largely safe tools, Extract Method can be unwieldy. Here are some warning signs that you’re trying to Extract Method in the wrong place:

Too many params

If Extract Method introduces more than three params into the new method, you might have selected too much or too little code.

• That’s a sign there are too many dependencies included in the code chunk you’re abstracting. Too many dependencies, and the code is still hard to test.
• Try to play around with selecting a different block of code and watch how that parameter list changes

Obvious syntax errors

Refactoring tools aren’t perfect, and we can catch a lot of their misfires with a good linting tool.

Unclear starting point

If your newly refactored code doesn’t help you find a starting point for a test, you can:

• Try narrowing it down to a smaller subset of code and get it in a test harness
• Extract another method and start from the top!

More reading

• The Usability (or Not) of Refactoring Tools