Refactoring for Interviews – Nickolas Means

Refactoring for Interviews

Technical interviews are an important part of the developer hiring process almost everywhere, but how you choose to conduct them has a lot to do with what kinds of information you can learn from them.

Whiteboard interviews put the candidate in an incredibly intimidating situation and ask them to write code without the benefit of any of the tools they use on a daily basis. They show you how good a candidate is at writing code on a whiteboard, but not much else.

Having the candidate build a from-scratch solution to meet a spec is a little better, but it only shows you how a candidate might tackle a greenfield project. Real-world developer tasks almost always involve working within the confines of an existing system.

I think you can actually learn the most about a candidate in the shortest amount of time by working through a refactoring exercise with them.

Let's take a look at a simple example! This class implements a few useful methods for scoring the children's dice game "Stuck in the Mud". In the game, you begin your turn by rolling 5 dice. You set aside any dice that come up 2 or 5 – they’re “stuck in the mud” – then add the sum of the remaining dice to your score. You keep re-rolling the scoring dice, and your turn ends once all of your dice are stuck in the mud.

class StuckInTheMudRoll

  def initialize(d1, d2, d3, d4, d5)
    @d1, @d2, @d3, @d4, @d5 = d1, d2, d3, d4, d5
  end

  def score
    score = 0
    score += @d1 unless @d1 == 2 || @d1 == 5
    score += @d2 unless @d2 == 2 || @d2 == 5
    score += @d3 unless @d3 == 2 || @d3 == 5
    score += @d4 unless @d4 == 2 || @d4 == 5
    score += @d5 unless @d5 == 2 || @d5 == 5
    return score
  end

  def end_of_turn?
    return false if @d1 != 2 and @d1 != 5
    return false if @d2 != 2 and @d2 != 5
    return false if @d3 != 2 and @d3 != 5
    return false if @d4 != 2 and @d4 != 5
    return false if @d5 != 2 and @d5 != 5
    return true
  end

  def self.dice_available_next_roll(d1, d2, d3, d4, d5)
    dice = [d1, d2, d3, d4, d5]
    total_dice = 5
    dice.each do |d|
      total_dice -= 1 if (d == 2 or d == 5)
    end
    return total_dice
  end

end

Because we want to see how our candidate approaches adding functionality to existing code, we want to give them a task. For this exercise, I'd point out that subsequent rolls might have less than 5 dice, but the class can only score a full five-dice roll.

One of the first questions I'm hoping I'll hear is "Are there any tests for this code?" And there are!

require './stuck_in_the_mud_roll.rb'
require 'minitest/autorun'

class TestStuckInTheMudRoll < Minitest::Test

  def test_dice_available_next_roll
    assert_equal 5, StuckInTheMudRoll.dice_available_next_roll(1,3,4,4,6)
    assert_equal 3, StuckInTheMudRoll.dice_available_next_roll(1,2,3,4,5)
    assert_equal 0, StuckInTheMudRoll.dice_available_next_roll(2,2,5,5,5)
  end

  def test_score
    mud = StuckInTheMudRoll.new(1,1,1,1,1)
    assert_equal 5, mud.score
 
    mud = StuckInTheMudRoll.new(1,1,1,2,5)
    assert_equal 3, mud.score

    mud = StuckInTheMudRoll.new(2,2,2,5,5)
    assert_equal 0, mud.score
  end

  def test_end_of_turn
    mud = StuckInTheMudRoll.new(1,2,3,4,5)
    assert_equal false, mud.end_of_turn?

    mud = StuckInTheMudRoll.new(2,2,5,5,5)
    assert_equal true, mud.end_of_turn?
  end
end

Asking about tests isn't a hard filter, but it does tell me if the candidate is used to relying on tests for safety when they change existing code.

If they don't ask, I might prompt them by saying something like "Oh, I forgot to mention, there are also tests if you'd find them helpful." I specifically don't want to make them feel like they've messed up, because I don't want them to be more nervous than they likely already are.

Next, I'm curious if they'll dive straight into changing the code , or if they'll look to refactor the class first to make the change easy.

If they dive right in, I'll ask a gently-prompting question like "Is there anything you might want to do first to make this change easier?"

One of the first things they're likely to notice is that the dice_available_next_roll method is a function implemented at the class level, while the rest of the class is instance methods.

That mismatch is on purpose, not to see if the candidate knows the difference, but to get us into a conversation about code design. It seems simple, but I've gleaned significant information about a candidate's grasp of object-oriented design principles this way.

def self.dice_available_next_roll(d1, d2, d3, d4, d5)
  dice = [d1, d2, d3, d4, d5]
  total_dice = 5
  dice.each do |d|
    total_dice -= 1 if (d == 2 || d == 5)
  end
  return total_dice
end

Let's say they choose to move forward with an instance-based implementation. Converting this class function to an instance method is straightforward.

def dice_available_next_roll
  dice = [@d1, @d2, @d3, @d4, @d5]
  total_dice = 5
  dice.each do |d|
    total_dice -= 1 if (d == 2 || d == 5)
  end
  return total_dice
end

Next, they'll likely begin reducing the repetition in the class. Looking at the score method , there's five lines that are basically the same.

def score
  score = 0
  score += @d1 unless @d1 == 2 || @d1 == 5
  score += @d2 unless @d2 == 2 || @d2 == 5
  score += @d3 unless @d3 == 2 || @d3 == 5
  score += @d4 unless @d4 == 2 || @d4 == 5
  score += @d5 unless @d5 == 2 || @d5 == 5
  return score
  end

One of the things I'm watching for as they refactor is how far they're willing to stray from passing tests. I'm hoping to see the small, incremental changes that demonstrate their refactoring experience.

In this case, the candidate would probably start by putting the dice in an array

def score
  dice = [@d1, @d2, @d3, @d4, @d5]
  score = 0
  score += @d1 unless @d1 == 2 || @d1 == 5
  score += @d2 unless @d2 == 2 || @d2 == 5
  score += @d3 unless @d3 == 2 || @d3 == 5
  score += @d4 unless @d4 == 2 || @d4 == 5
  score += @d5 unless @d5 == 2 || @d5 == 5
  return score
end

and then rebuild the body of the method in an iterator.

def score
  dice = [@d1, @d2, @d3, @d4, @d5]
  dice.inject(0) do |sum, d| 
    sum += d unless d == 2 || d == 5
    sum
  end
end

First, sticking with the original logic and checking the tests.

$ ruby test_stuck_in_the_mud.rb
Run options: --seed 12301

# Running:

...

Finished in 0.001017s, 2949.8530 runs/s, 7866.2747 assertions/s.
3 runs, 8 assertions, 0 failures, 0 errors, 0 skips

And then, to simplify the method, perhaps using reject! to filter the dice they don't want before calculating the sum.

def score
  dice = [@d1, @d2, @d3, @d4, @d5]
  dice.reject!{|d| d == 2 || d == 5}
  dice.inject(0){|sum, d| sum += d}
end

They could use the same technique to refactor end_of_turn? as well:

def end_of_turn?
  return false if @d1 != 2 and @d1 != 5
  return false if @d2 != 2 and @d2 != 5
  return false if @d3 != 2 and @d3 != 5
  return false if @d4 != 2 and @d4 != 5
  return false if @d5 != 2 and @d5 != 5
  return true
end

Adding the dice array, rejecting the 2's and 5's, and then checking to see if there are no dice left to roll:

def end_of_turn?
  dice = [@d1, @d2, @d3, @d4, @d5]
  dice.reject!{|d| d == 2 || d == 5}
  dice.size == 0
end

At this point, all three of the methods are iterating over a dice array, but only two of them are using reject! to filter out the dice that are stuck in the mud. I'd really like to see the candidate apply the same pattern to dice_available_next_roll to make things look as similar as possible before beginning extraction.

def dice_available_next_roll
  dice = [@d1, @d2, @d3, @d4, @d5]
  total_dice = 5
  dice.each do |d|
    total_dice -= 1 if (d == 2 || d == 5)
  end
  return total_dice
end

And doing so is fairly straightforward:

def dice_available_next_roll
  dice = [@d1, @d2, @d3, @d4, @d5]
  dice.reject!{|d| d == 2 || d == 5}
  dice.size
end

Now, taking a look at our class, the candidate has gotten us to where the first two lines of each method are identical:

class StuckInTheMudRoll

  def initialize(d1, d2, d3, d4, d5)
    @d1, @d2, @d3, @d4, @d5 = d1, d2, d3, d4, d5
  end

  def score
    dice = [@d1, @d2, @d3, @d4, @d5]
    dice.reject!{|d| d == 2 || d == 5}
    dice.inject(0){|sum, d| sum += d}
  end

  def end_of_turn?
    dice = [@d1, @d2, @d3, @d4, @d5]
    dice.reject!{|d| d == 2 || d == 5}
    dice.size == 0
  end

  def dice_available_next_roll
    dice = [@d1, @d2, @d3, @d4, @d5]
    dice.reject!{|d| d == 2 || d == 5}
    dice.size
  end
end

That symmetry lets them extract those two lines to the initializer, and adding an attr_reader for dice avoids the need to change dice to an instance variable in each method:

class StuckInTheMudRoll

  attr_reader :dice
  
  def initialize(d1, d2, d3, d4, d5)
    @dice = [d1, d2, d3, d4, d5]
    @dice.reject!{|d| d == 2 || d == 5}
  end

Then they can delete those lines from all three methods, making the class quite clean:

class StuckInTheMudRoll

  attr_reader :dice
  
  def initialize(d1, d2, d3, d4, d5)
    @dice = [d1, d2, d3, d4, d5]
    @dice.reject!{|d| d == 2 || d == 5}
  end

  def score
    dice.inject(0){|sum, d| sum += d}
  end

  def end_of_turn?
    dice.size == 0
  end

  def dice_available_next_roll
    dice.size
  end
end

The next step they'll need to tackle is updating the class to support rolls of less than 5 dice. At this point, I'm curious to see if they add a failing test before they start modifying the class, like this.

def test_score
  mud = StuckInTheMudRoll.new(1,1,1,1,1)
  assert_equal 5, mud.score

  mud = StuckInTheMudRoll.new(1,1,1,2,5)
  assert_equal 3, mud.score

  mud = StuckInTheMudRoll.new(2,2,2,5,5)
  assert_equal 0, mud.score
  
  mud = StuckInTheMudRoll.new(1,2,1,1)
  assert_equal 3, mud.score
end

and that get them the failure we expect to see:

$ ruby test_stuck_in_the_mud_roll.rb
Run options: --seed 61581

# Running:

..E

Error:
TestStuckInTheMudRoll#test_score:
ArgumentError: wrong number of arguments (given 4, expected 5)
    /Users/nmeans/projects/tapas/stuck_in_the_mud_roll.rb:5:in `initialize'
    test_stuck_in_the_mud_roll.rb:27:in `new'
    test_stuck_in_the_mud_roll.rb:27:in `test_score'

But because of the refactoring our candidate has already done, it's pretty simple for them to modify the class to get the test passing. One approach would be to use a splat to allow the initializer to accept a variable number of arguments .

def initialize(*dice)
  @dice = dice
  @dice.reject!{|d| d == 2 || d == 5}
end

Given how that shortens the assignment to the instance variable, our candidate might choose to move the call to reject onto the same line, removing the bang in the process.

def initialize(*dice)
  @dice = dice.reject{|d| d == 2 || d == 5}
end

That gets the tests passing again.

$ ruby test_stuck_in_the_mud_roll.rb
Run options: --seed 36195

# Running:

...

Finished in 0.000904s, 3318.5836 runs/s, 9955.7508 assertions/s.
3 runs, 9 assertions, 0 failures, 0 errors, 0 skips

And here's the final version of our candidate's refactoring exercise:

class StuckInTheMudRoll

  attr_reader :dice
  
  def initialize(*dice)
    @dice = dice.reject{|d| d == 2 || d == 5}
  end

  def score
    dice.inject(0){|sum, d| sum += d}
  end

  def end_of_turn?
    dice.size == 0
  end

  def dice_available_next_roll
    dice.size
  end
end

When I'm conducting a technical interview, my goal is always to make the candidate as comfortable as possible. I want to see them at their best, so I do everything that I can to set them up for success. Using a refactoring exercise is part of that. I find that refactoring exercises take away the anxiety of a blank whiteboard or empty code file and replace it with the relative comfort of reading code and making it better. They also give the candidate the opportunity to show us how they approach understanding and modifying existing code, the very thing they'll spend most of their time on in all but the newest of projects, giving us incredibly useful information to use in our hiring decision. This approach has helped me with every hire I've made, and I hope it will help you as well!