Kerry Buckley

What’s the simplest thing that could possibly go wrong?

Naresh Jain’s refactoring teaser

leave a comment

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:

PLAIN TEXT
RUBY:
  1. #!/usr/bin/env ruby
  2. #
  3. #See http://blogs.agilefaqs.com/2009/07/08/refactoring-teaser-part-1/
  4.  
  5. require 'spec'
  6.  
  7. module StringExtensions
  8.   REGEX_TO_SPLIT_ALONG_WHITESPACES = /\s+/
  9.  
  10.   def phrases(number)
  11.     list_of_keywords = ""
  12.     count = 0
  13.     strings = split(REGEX_TO_SPLIT_ALONG_WHITESPACES)
  14.     all_strings = single_double_triple_words(strings)
  15.     size = all_strings.size
  16.     all_strings.each do |phrase|
  17.       break if count == number
  18.       list_of_keywords += "'" + phrase + "'"
  19.       count += 1
  20.       if (count <size && count <number)
  21.         list_of_keywords += ", "
  22.       end
  23.     end
  24.     return list_of_keywords
  25.   end
  26.  
  27.   private
  28.  
  29.   def single_double_triple_words(strings)
  30.     all_strings = []
  31.     num_words = strings.size
  32.  
  33.     return all_strings unless has_enough_words(num_words)
  34.  
  35.     # Extracting single words. Total size of words == num_words
  36.  
  37.     # Extracting single-word phrases.
  38.     (0...num_words).each do |i|
  39.       all_strings <<strings[i]
  40.     end
  41.  
  42.     # Extracting double-word phrases
  43.     (0...num_words - 1).each do |i|
  44.       all_strings <<"#{strings[i]} #{strings[i + 1]}"
  45.     end
  46.  
  47.     # Extracting triple-word phrases
  48.     (0...num_words - 2).each do |i|
  49.       all_strings <<"#{strings[i]} #{strings[i + 1]} #{strings[i + 2]}"
  50.     end
  51.     return all_strings
  52.   end
  53.  
  54.   def has_enough_words(num_words)
  55.     num_words>= 3
  56.   end
  57. end
  58.  
  59. String.send(:include, StringExtensions)
  60.  
  61. describe StringExtensions do
  62.   it 'finds all phrases' do
  63.     'Hello World Ruby'.phrases(6).should == "'Hello', 'World', 'Ruby', 'Hello World', 'World Ruby', 'Hello World Ruby'"
  64.   end
  65.  
  66.   it 'returns all phrases when asked for more than exist' do
  67.     'Hello World Ruby'.phrases(10).should == "'Hello', 'World', 'Ruby', 'Hello World', 'World Ruby', 'Hello World Ruby'"
  68.   end
  69.  
  70.   it 'returns the first n phrases when asked for fewer than exist' do
  71.     'Hello World Ruby'.phrases(4).should == "'Hello', 'World', 'Ruby', 'Hello World'"
  72.   end
  73.  
  74.   it 'returns the first word when asked for one phrase' do
  75.     'Hello World Ruby'.phrases(1).should == "'Hello'"
  76.   end
  77. 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):

PLAIN TEXT
RUBY:
  1. def phrases(number)
  2.   strings = split(REGEX_TO_SPLIT_ALONG_WHITESPACES)
  3.   all_strings = single_double_triple_words(strings)
  4.   all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
  5. 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:

PLAIN TEXT
RUBY:
  1. def phrases(number)
  2.   words = split
  3.   all_strings = single_double_triple_words(words)
  4.   all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
  5. 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):

PLAIN TEXT
RUBY:
  1. module StringExtensions
  2.   def phrases(number)
  3.     words = split
  4.     all_strings = words
  5.     all_strings += double_triple_words(words)
  6.     all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
  7.   end
  8.  
  9.   private
  10.  
  11.   def double_triple_words(strings)
  12.     all_strings = []
  13.     num_words = strings.size
  14.  
  15.     return all_strings unless has_enough_words(num_words)
  16.  
  17.     # Extracting double-word phrases
  18.     (0...num_words - 1).each do |i|
  19.       all_strings <<"#{strings[i]} #{strings[i + 1]}"
  20.     end
  21.  
  22.     # Extracting triple-word phrases
  23.     (0...num_words - 2).each do |i|
  24.       all_strings <<"#{strings[i]} #{strings[i + 1]} #{strings[i + 2]}"
  25.     end
  26.     return all_strings
  27.   end
  28.  
  29.   def has_enough_words(num_words)
  30.     num_words>= 3
  31.   end
  32. 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:

PLAIN TEXT
RUBY:
  1. module StringExtensions
  2.   def phrases(number)
  3.     words = split
  4.     all_strings = words
  5.     all_strings += double_words(words)
  6.     all_strings += triple_words(words)
  7.     all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
  8.   end
  9.  
  10.   private
  11.  
  12.   def double_words(strings)
  13.     all_strings = []
  14.     num_words = strings.size
  15.  
  16.     # Extracting double-word phrases
  17.     (0...num_words - 1).each do |i|
  18.       all_strings <<"#{strings[i]} #{strings[i + 1]}"
  19.     end
  20.     return all_strings
  21.   end
  22.  
  23.   def triple_words(strings)
  24.     all_strings = []
  25.     num_words = strings.size
  26.  
  27.     (0...num_words - 2).each do |i|
  28.       all_strings <<"#{strings[i]} #{strings[i + 1]} #{strings[i + 2]}"
  29.     end
  30.     return all_strings
  31.   end
  32. 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):

PLAIN TEXT
RUBY:
  1. def double_words(strings)
  2.   all_strings = []
  3.  
  4.   # Extracting double-word phrases
  5.   (0...strings.size - 1).each do |i|
  6.     all_strings <<"#{strings[i]} #{strings[i + 1]}"
  7.   end
  8.   return all_strings
  9. 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:

PLAIN TEXT
RUBY:
  1. def double_words(strings)
  2.   extract_phrases(strings, 2)
  3. end
  4.  
  5. def triple_words(strings)
  6.   extract_phrases(strings, 3)
  7. end
  8.  
  9. def extract_phrases(strings, number_of_words)
  10.   result = []
  11.   (0...strings.size - number_of_words + 1).each do |i|
  12.     phrase = strings[i, number_of_words].join(' ')
  13.     result <<phrase
  14.   end
  15.   result
  16. 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:

PLAIN TEXT
RUBY:
  1. def phrases(number)
  2.   words = split
  3.   all_strings = words
  4.   all_strings += extract_phrases(words, 2)
  5.   all_strings += extract_phrases(words, 3)
  6.   all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
  7. end

Rather than hardcoding the calls for two and three words, I changed it Δ to use a loop:

PLAIN TEXT
RUBY:
  1. def phrases(number)
  2.   words = split
  3.   all_strings = words
  4.   (2..words.size).each do |number_of_words|
  5.     all_strings += extract_phrases(words, number_of_words)
  6.   end
  7.   all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
  8. end

I decided this was the point to put the optimisation back Δ and stop looking for phrases once we had enough:

PLAIN TEXT
RUBY:
  1. def phrases(number)
  2.   words = split
  3.   all_strings = words
  4.   (2..words.size).each do |number_of_words|
  5.     break if all_strings.length>= number
  6.     all_strings += extract_phrases(words, number_of_words)
  7.   end
  8.   all_strings[0, number].map {|s| "'#{s}'"}.join(', ')
  9. 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:

PLAIN TEXT
RUBY:
  1. def extract_phrases(strings, phrase_length)
  2.   (0...strings.size - phrase_length + 1).map do |i|
  3.     strings[i, phrase_length].join(' ')
  4.   end
  5. 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:

PLAIN TEXT
RUBY:
  1. module StringExtensions
  2.   def phrases(number)
  3.     words = split
  4.     all_strings = words
  5.     (2..words.size).each do |phrase_length|
  6.       break if all_strings.length>= number
  7.       all_strings += extract_phrases(words, phrase_length)
  8.     end
  9.     format_output(all_strings[0, number])
  10.   end
  11.  
  12.   private
  13.  
  14.   def extract_phrases(words, phrase_length)
  15.     (0...words.size - phrase_length + 1).map do |i|
  16.       words[i, phrase_length].join(' ')
  17.     end
  18.   end
  19.  
  20.   def format_output(phrases)
  21.     phrases.map {|s| "'#{s}'"}.join(', ')
  22.   end
  23. end

Written by Kerry

July 17th, 2009 at 8:03 am

Posted in Ruby

Tagged with , ,

«
»

Leave a Reply