Return to Answer

added 1362 characters in body

edited Jan 10, 2014 at 21:24

3.3k
1
18
38

The sequence aeiou might deserve to be a constant:

VOWEL = 'aeiou' ... if word[0] =~ /[#{VOWEL}]/i ... when /[#{VOWEL}]/ ... consonants = word.slice!(/[^#{VOWEL}]*/)

Handling of capitalization can be handled by, at the beginning of the function, noticing whether or not the word is capitalized, and then downcasing it:

is_capitalized = word =~ /^A-Z/ word = word.downcase

and at the end, capitalizing it again if needed.

word = word.capitalized if is_capitalized

This removes considerations of case from the rest of the function.

Expressions like word[0] =~ /.../ can be replaced with word =~ /^.../. Similarly, word[1] =~ /.../ can be replaced with `word =~ /...$/

Taken altogether, these suggestions yield a #translate_word more like this:

 def translate_word(word) is_capitalized = word =~ /^[A-Z]/ word = word.downcase if (word =~ /^#{VOWEL}/i) case word when /#{VOWEL}$/ word += "yay" when /y$/ word += "nay" else word += "ay" end else consonants = word.slice!(/^[^#{VOWEL}]*/) word += consonants + "ay" end word = word.capitalize if is_capitalized word end

The sequence aeiou might deserve to be a constant:

VOWEL = 'aeiou' ... if word[0] =~ /[#{VOWEL}]/i ... when /[#{VOWEL}]/ ... consonants = word.slice!(/[^#{VOWEL}]*/)

Handling of capitalization can be handled by, at the beginning of the function, noticing whether or not the word is capitalized, and then downcasing it:

is_capitalized = word =~ /^A-Z/ word = word.downcase

and at the end, capitalizing it again if needed.

word = word.capitalized if is_capitalized

This removes considerations of case from the rest of the function.

Expressions like word[0] =~ /.../ can be replaced with word =~ /^.../. Similarly, word[1] =~ /.../ can be replaced with `word =~ /...$/

Taken altogether, these suggestions yield a #translate_word more like this:

 def translate_word(word) is_capitalized = word =~ /^[A-Z]/ word = word.downcase if (word =~ /^#{VOWEL}/i) case word when /#{VOWEL}$/ word += "yay" when /y$/ word += "nay" else word += "ay" end else consonants = word.slice!(/^[^#{VOWEL}]*/) word += consonants + "ay" end word = word.capitalize if is_capitalized word end

Source Link

answered Jan 10, 2014 at 21:12

Wayne Conrad

3.3k
1
18
38

Good marks for:

Formatting
Naming
Each spec tests just one thing
Coverage of edge cases such as empty string.

I think I would change the name from PigLatin to PigLatinTranslator. I can't think of a "pig latin" as a thing.

Was the API required by the challenge? If not, I'd consider simplifying it. There's not much reason for the translator to have any state. You can simplify it if the #translate method takes the string to be translated. You won't need the accessors or the #initialize method.

That #translate returns nil if the phrase is an empty string is a little surprising. I expect it to return an empty string.

Instead of splitting on white space, consider using gsub to scan for words and replace them. If you do that, you can remove all of #translate_word's special handling of punctuation:

 def translate return nil if @phrase.empty? @phrase.gsub(/\w+/) do |word| translate_word(word) end end

return word at the end of #translate_word can be simply word. The only reason to use the return keyword is for a return before the end of the method.