Pig Latin Translator - follow-up

Question

About a week ago, I put up this code and got some really good help. I've modified it a bit and was seeking if someone would review my second iteration. Original can be found here.

Here's the new stuff. Basically, I stuck it all in a module, added support for the option of a dash or no dash. And gave it the ability to work with sentences.

module PigLatin
class Translator
#how all the words end
ENDING = "ay"

def initialize (options = {:dash => true})
  @dash = options[:dash]
end

def translate (phrase)
  sentences = phrase.split(".")
  translated_sentences = sentences.map do |sentence|
    translation(sentence) + "."
  end
  translated_sentences.join(" ")
end

private

def translation (phrase)
  words = phrase.split
  translated_words = words.map do |word|
    if vowel_is_first(word)
      translate_with_vowel(word)
    else
      translate_with_consonant(word)
    end
  end
  translated_phrase = translated_words.join(" ")
  if @dash == false
    translated_phrase = translated_phrase.delete("-")
  end
  translated_phrase.capitalize
end

def vowel_is_first(word)
  return true if word[0] =~ /a|e|i|o|u|A|E|I|O|U/
  return false
end

def translate_with_vowel(word)
  "#{word}-#{"w"+ENDING}"
end

def translate_with_consonant(word)
  return "#{word[1]}-#{word[0]+ENDING}" if word.size == 2
  second_segment, first_segment = split(word)
  return "#{first_segment}-#{second_segment+ENDING}"
end

def split(word)
  split_location = word =~ /a|e|i|o|u/
  second_segment = word[0,split_location]
  first_segment = word[split_location,word.size]
  return second_segment, first_segment
end
end

class InputAcceptor

def initialize input, options
  @options = options
  @input = input
end

def do_it
  pig_latin = Translator.new(@options)
  pig_latin.translate(@input)
end

end


end
  j = PigLatin::InputAcceptor.new("Hello. You. Guy.",{:dash => false})
  p j.do_it

Answer 1

Some points in addition to what checkorbored pointed out:

1. You should use character classes and the i flag for your regexen. E.g. instead of /a|e|i|o|u|A|E|I|O|U/ you can write /[aeiou]/i.

2. You should get rid of the explicit return statements where not necessary.

3. return foo if bar
   return baz
   

   should really be written as

   if bar
     foo
   else
     baz
   end
   

   Except if foo and baz are booleans in which case the whole thing can just be replaced by bar or !bar.

4. def initialize (options = {:dash => true})
     @dash = options[:dash]
   end
   

   In this case it doesn't matter because there's only one option in your options hash, but the usual idiom to implement keyword arguments with defaults looks like this:

   def initialize (options = {})
     options = {:dash => true}.merge(options)
     @dash = options[:dash]
   end
   

   The reason that (options = {defaults}) is generally not used, is that it doesn't allow the user to specify some options, but use the default for others. So if you do def bla(options = {:foo => 42, :bar => 23}) and the user calls bla(:bar => 24) the value for :foo will be nil, not 42. That doesn't happen if you use merge instead.

Answer 2

I think this is a great way to work on your skills, and I applaud your perseverance here. In particular, it is important to recognize that building software is incremental, and ideally there is a tight loop of development, review, re-factoring, re-review, etc.

Here are some comments from me:

def vowel_is_first(word)
   return true if word[0] =~ /a|e|i|o|u|A|E|I|O|U/
   return false
end

In general, it's not standard style to use strings like arrays, and you can essentially always find a different way to express what you need without doing that. Additionally, rather than using alternation (|) in the regular expression, you should use a character class - simpler, clearer, and more atomic. My suggestion:

def vowel_is_first(word)
    return word =~ /^[aeiouAEIOU]/
end

The caret (^) matches at the beginning of a string (or the beginning of a line, but we aren't dealing with multi-line strings), and then our character class matches one occurence of any character inside the square brackets. This will return a non-nil value if the first character is a vowel, and nil otherwise.

Moving on to:

"#{word}-#{"w"+ENDING}"

You dont need to put literal strings within interpolation, and concatenating strings with + should generally be a no-no. This would be clearer as:

"#{word}-w#{ENDING}"

Next,

return "#{word[1]}-#{word[0]+ENDING}" if word.size == 2

We need to do away with these array-style references again. This is a perfect spot for a gsub:

return word.gsub(/^(.)(.)$/, "\\2-\\1#{ENDING}") if word.size == 2

We capture two single characters, which we are guaranteed due to the conditional. Then, we reverse their order, putting a dash between them and ending with, well, our ending.

With regard to the most complicated part of the code; viz. translate_with_consonant for words with longer than 2 letters - which basically means the split method which you wrote. I agree with sepp2k that you could use the built-in split with a clever regular expression to split on, but I think the whole thing can be done in a gsub again. You should try to come up with it yourself.

return word.gsub(/^([^aeiouAEIOU]+)([aeiouAEIOU][A-Za-z]*)/, "\\2-\\1ay")

Weird, using backticks and spoiler together makes it less spoiler. Oh well.

Explanation:

We look for a non-vowel to begin the string (inside of a character class, the caret means to take the inverse; look for anything but what is listed). If we get more than one non-vowel, that's OK (+ means one or more), but we capture the second part starting with the first vowel, and continuing through the last letter.

The great thing about this regular expression is that you can use it for the entirety of translate_with_consonant - the two-letter words fit right in. And no need for splitting, joining, concatenating, or another method. Additionally, it works better if I pass you an exclamation point or a comma - try it!

Finally, I would say that you don't need the InputAcceptor class at all. Keep the interface simple, as sepp2k suggested, and make the translation happen in one method call.

I look forward to seeing more!

Answer 3

My 2-cents:

No spaces between function name and parenthesis: def function(arg1, args)

No if something == true or if something == false, directly if something or if !something.

You can continue writing code after an end.

 def translate(phrase)
   translated_sentences = phrase.split(".").map do |sentence|
     translation(sentence) + "."
   end.join(" ")
 end

Don't reuse/update variables, create new ones or write it in other way:

translated_phrase = translated_words.join(" ")
if @dash == false
  translated_phrase = translated_phrase.delete("-")
end

To:

translated_phrase0 = translated_words.join(" ")
translated_phrase = @dash ? translated_phrase0 : translated_phrase0.delete("-")