Refactoring for Interviews – Nickolas Means
There are a lot of different opinions about the best way to conduct a technical interview. Our guest chef today has an approach that avoids some of the pitfalls of whiteboard coding and fizzbuzz-style toy problems. Instead, it challenges candidates to demonstrate concrete abilities on a very real-world challenge.
Video transcript & code
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!
Responses