Naresh Jain recently posted a refactoring teaser. The original code was in Java, but I thought I’d have a go at refactoring it in Ruby instead. I deliberately didn’t look at Naresh’s solution beforehand, so mine goes in a rather different direction.
My code’s here (with the specs in the same file for simplicity), and you can step through the history to see the state at each step of the refactoring. The links in the text below take you to the relevant version of the file, and the Δs next to them link to the diffs.
Firstly, I converted the code to Ruby, and made sure the tests (which I converted to RSpec) still passed. It’s a fairly straight conversion, although I made a couple of changes while I was at it – mainly turning it into a module which I mixed into String. I changed the name of the method to phrases, to avoid conflicting with the built-in String#split. The regular expression to split on is much simpler too, because Ruby didn’t understand the original, and I had no idea what it was supposed to do anyway.
Here’s my initial Ruby version:
#!/usr/bin/env ruby
#
#See http://blogs.agilefaqs.com/2009/07/08/refactoring-teaser-part-1/
require 'spec'
module StringExtensions
REGEX_TO_SPLIT_ALONG_WHITESPACES = /\s+/
def phrases(number)
list_of_keywords = ""
count = 0
strings = split(REGEX_TO_SPLIT_ALONG_WHITESPACES)
all_strings = single_double_triple_words(strings)
size = all_strings.size
all_strings.each do |phrase|
break if count == number
list_of_keywords += "'" + phrase + "'"
count += 1
if (count < size && count < number)
list_of_keywords += ", "
end
end
return list_of_keywords
end
private
def single_double_triple_words(strings)
all_strings = []
num_words = strings.size
return all_strings unless has_enough_words(num_words)
# Extracting single words. Total size of words == num_words
# Extracting single-word phrases.
(0...num_words).each do |i|
all_strings << strings[i]
end
# Extracting double-word phrases
(0...num_words - 1).each do |i|
all_strings << "#{strings[i]} #{strings[i + 1]}"
end
# Extracting triple-word phrases
(0...num_words - 2).each do |i|
all_strings << "#{strings[i]} #{strings[i + 1]} #{strings[i + 2]}"
end
return all_strings
end
def has_enough_words(num_words)
num_words >= 3
end
end
String.send(:include, StringExtensions)
describe StringExtensions do
it 'finds all phrases' do
'Hello World Ruby'.phrases(6).should == "'Hello', 'World', 'Ruby', 'Hello World', 'World Ruby', 'Hello World Ruby'"
end
it 'returns all phrases when asked for more than exist' do
'Hello World Ruby'.phrases(10).should == "'Hello', 'World', 'Ruby', 'Hello World', 'World Ruby', 'Hello World Ruby'"
end
it 'returns the first n phrases when asked for fewer than exist' do
'Hello World Ruby'.phrases(4).should == "'Hello', 'World', 'Ruby', 'Hello World'"
end
it 'returns the first word when asked for one phrase' do
'Hello World Ruby'.phrases(1).should == "'Hello'"
end
end
I didn’t change the specs at all during the refactoring (because I didn’t change the API or add any new public methods or classes), and made sure they all passed at each step.
The first thing Δ I changed was to simplify that big iterator in phrases that loops through the list of phrases, formatting the output string. Basically all this does is to put each phrase in quotes, then stitch them all together separated by a comma and a space. The first of those tasks is a simple map, and the second is a join. The whole method collapses down to this (ruby methods automatically return the result of their last statement):
def phrases(number)
strings = split(REGEX_TO_SPLIT_ALONG_WHITESPACES)
all_strings = single_double_triple_words(strings)
all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
end
Next Δ I remembered that by default String#split splits at whitespace anyway, so I did away with the regular expression. Then Δ I renamed the strings variable to words to make its purpose a little clearer, leaving the phrases method looking like this:
def phrases(number)
words = split
all_strings = single_double_triple_words(words)
all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
end
The section of single_double_triple_words that extracted the single words seemed redundant, as we already had that list – the original words. I removed it Δ, and initialised all_strings to the word list instead (not forgetting to rename single_double_triple_words to match its new behaviour):
module StringExtensions
def phrases(number)
words = split
all_strings = words
all_strings += double_triple_words(words)
all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
end
private
def double_triple_words(strings)
all_strings = []
num_words = strings.size
return all_strings unless has_enough_words(num_words)
# Extracting double-word phrases
(0...num_words - 1).each do |i|
all_strings << "#{strings[i]} #{strings[i + 1]}"
end
# Extracting triple-word phrases
(0...num_words - 2).each do |i|
all_strings << "#{strings[i]} #{strings[i + 1]} #{strings[i + 2]}"
end
return all_strings
end
def has_enough_words(num_words)
num_words >= 3
end
end
That has_enough_words method seemed a bit odd – particularly the way it was only called once, rather than after extracting each set of phrases. I decided it was probably a premature and incomplete attempt at optimisation, and removed it Δ for now.
My next target was the duplication in the blocks that calculate double- and triple-word phrases. First Δ I extracted them into separate methods:
module StringExtensions
def phrases(number)
words = split
all_strings = words
all_strings += double_words(words)
all_strings += triple_words(words)
all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
end
private
def double_words(strings)
all_strings = []
num_words = strings.size
# Extracting double-word phrases
(0...num_words - 1).each do |i|
all_strings << "#{strings[i]} #{strings[i + 1]}"
end
return all_strings
end
def triple_words(strings)
all_strings = []
num_words = strings.size
(0...num_words - 2).each do |i|
all_strings << "#{strings[i]} #{strings[i + 1]} #{strings[i + 2]}"
end
return all_strings
end
end
I decided that the num_words variable in the two new methods wasn't really necessary (it was only used once, and I think strings.size expresses intent perfectly clearly), so I inlined it Δ (and the same in triple_words):
def double_words(strings)
all_strings = []
# Extracting double-word phrases
(0...strings.size - 1).each do |i|
all_strings << "#{strings[i]} #{strings[i + 1]}"
end
return all_strings
end
Most of the code in double_words and triple_words was obviously very similar, so I created Δ a general extract_phrases method, and called it from both. The new method uses the start position and length version of Array#[] to extract the appropriate number of words, then Array#join to string them together separated by spaces:
def double_words(strings)
extract_phrases(strings, 2)
end
def triple_words(strings)
extract_phrases(strings, 3)
end
def extract_phrases(strings, number_of_words)
result = []
(0...strings.size - number_of_words + 1).each do |i|
phrase = strings[i, number_of_words].join(' ')
result << phrase
end
result
end
At this point double_words and triple_words have become just dumb wrappers around extract_phrases, so I removed them Δ and just called extract_phrases directly:
def phrases(number)
words = split
all_strings = words
all_strings += extract_phrases(words, 2)
all_strings += extract_phrases(words, 3)
all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
end
Rather than hardcoding the calls for two and three words, I changed it Δ to use a loop:
def phrases(number)
words = split
all_strings = words
(2..words.size).each do |number_of_words|
all_strings += extract_phrases(words, number_of_words)
end
all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
end
I decided this was the point to put the optimisation back Δ and stop looking for phrases once we had enough:
def phrases(number)
words = split
all_strings = words
(2..words.size).each do |number_of_words|
break if all_strings.length >= number
all_strings += extract_phrases(words, number_of_words)
end
all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
end
I decided that number_of_words wasn't particularly clear, so changed it Δ to phrase_length, then Δ made the iterator in extract_phrases more ruby-like by using map:
def extract_phrases(strings, phrase_length)
(0...strings.size - phrase_length + 1).map do |i|
strings[i, phrase_length].join(' ')
end
end
I then noticed that I hadn't been consistent in changing strings to words, so fixed that Δ.
Lastly Δ, I decided that even though it was only one line, the code that formats the output deserved pulling out into a method.
Here's my final version of the code:
module StringExtensions
def phrases(number)
words = split
all_strings = words
(2..words.size).each do |phrase_length|
break if all_strings.length >= number
all_strings += extract_phrases(words, phrase_length)
end
format_output(all_strings[0, number])
end
private
def extract_phrases(words, phrase_length)
(0...words.size - phrase_length + 1).map do |i|
words[i, phrase_length].join(' ')
end
end
def format_output(phrases)
phrases.map {|s| "'#{s}'"}.join(', ')
end
end