2
\$\begingroup\$

I've just complete a Pig Latin translator as a code kata and I was hoping someone would review it for me.

Here is the kata:

PigLatin Kata Create a PigLatin class that is initialized with a string - detail: The string is a list of words separated by spaces: 'hello world' - detail: The string is accessed by a method named phrase - detail: The string can be reset at any time without re-initializing - example: PigLatin.new('hello world') completed (Y|n): Translate Method Create a translate method that translates the phrase from english to pig-latin. - detail: The method will return a string. - detail: The empty string will return nil. - example: "" translates to nil completed (Y|n): Translate words that start with vowels. - detail: Append "ay" to the word if it ends in a consonant. - example: "ask" translates to "askay" - detail: Append "yay" to the word if it ends with a vowel. - example: "apple" translates to "appleyay" - detail: Append "nay" to the word if it ends with "y". - example: "any" translates to "anynay" completed (Y|n): Translate a word that starts with a single consonant. - detail: Removing the consonant from the front of the word. - detail: Add the consonant to the end of the word. - detail: Append 'ay' to the resulting word. - example: "hello" translates to "ellohay" - example: "world" translates to "orldway" completed (Y|n): Translate words that start with multiple consonants. - detail: Remove all leading consonants from the front of the word. - detail: Add the consonants to the end of the word. - detail: Append 'ay' to the resulting word. - example: "known" translates to "ownknay" - example: "special" translates to "ecialspay" completed (Y|n): Support any number of words in the phrase. - example: "hello world" translates to "ellohay orldway" - detail: Each component of a hyphenated word is translated seperately. - example: "well-being" translates to "ellway-eingbay" completed (Y|n): Support capital letters. - detail: If a word is captalized, the translated word should be capitalized. - example: "Bill" translates to "Illbay" - example: "Andrew" translates to "Andreway" completed (Y|n): Retain punctuation. - detail: Punctuation marks should be retained from the source to the translated string - example: "fantastic!" tranlates to "anfasticfay!" - example: "Three things: one, two, three." translates to "Eethray ingsthay: oneyay, otway, eethray." completed (Y|n): Congratulations! - Create a PigLatin class that is initialized with a string 00:12:52 - Create a translate method that translates the phrase from english to p 00:03:00 - Translate words that start with vowels. 00:08:56 - Translate a word that starts with a single consonant. 00:04:32 - Translate words that start with multiple consonants. 00:08:08 - Support any number of words in the phrase. 00:25:19 - Support capital letters. 00:05:05 - Retain punctuation. 00:17:00 ---------------------------------------------------------------------- Total Time taking PigLatin kata: 01:24:52

And my specs:

require 'spec_helper' require 'piglatin' describe PigLatin do let(:pig_latin) { PigLatin.new(string) } let(:string) { "hello world" } describe "new" do specify { expect { pig_latin }.to_not raise_error } end describe ".phrase" do subject { pig_latin.phrase } it { should eq("hello world") } it "can reset the phrase" do pig_latin pig_latin.phrase = "world hello" subject.should eq("world hello") end end describe ".translate" do subject { pig_latin.translate } its(:class) { should eq(String) } context "empty string" do let(:string) { "" } it { should eq(nil) } end context "words that start with vowels" do let(:string) { "ask" } it { should eq("askay") } context "and also ends with a vowel" do let(:string) { "apple" } it { should eq("appleyay") } end context "and ends with y" do let(:string) { "any" } it { should eq("anynay") } end end context "words that start with a single consonant" do let(:string) { "hello" } it { should eq("ellohay") } end context "words that start with multiple consonants" do context "known" do let(:string) { "known" } it { should eq("ownknay") } end context "special" do let(:string) { "special" } it { should eq("ecialspay") } end end context "multiple words" do let(:string) { "hello world" } it { should eq("ellohay orldway") } context "hyphenated words" do let(:string) { "well-being" } it { should eq("ellway-eingbay") } end end context "Capitalization" do context "Bill" do let(:string) { "Bill" } it { should eq("Illbay") } end context "Andrew" do let(:string) { "Andrew" } it { should eq("Andreway") } end end context "Retain Punctuation" do context "fantastic!" do let(:string) { "fantastic!" } it { should eq("antasticfay!") } end context "Three things: one, two, three." do let(:string) { "Three things: one, two, three." } it { should eq("Eethray ingsthay: oneyay, otway, eethray.") } end end end end

And, finally, the PigLatin class:

class PigLatin attr_accessor :phrase def phrase=(string) @phrase = string end alias :initialize :phrase= def translate return nil if (@phrase.empty?) word_array = @phrase.split(/\s/) word_array.collect! do |word| translate_word(word) end translated_phrase = word_array.join(" ") return translated_phrase end private def translate_word word words = word.split("-") words.collect! do |word| punctuation = word.slice!(/\W/) if (word[0] =~ /[aeiou]/i) case word[-1] when /[aeiou]/ word += "yay" when /y/ word += "nay" else word += "ay" end else consonants = word.slice!(/^[^aeiou]*/) word.capitalize! if (consonants.downcase!) word += consonants + "ay" end if (punctuation) word << punctuation end word end return words.join("-") end end
\$\endgroup\$

3 Answers 3

Reset to default
2
\$\begingroup\$

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.

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
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the notes. This is really helpful. Re: return word I like explicitly writing return. I know it's not necessary but -- maybe because I'm an old-school C guy -- it looks better to me. I use parenthesis and stuff too ;) Re: state, I was modeling the class off of String, etc... where you have state and then perform operations from there. It might make sense to subclass String and add a translate method to out put pig latin ... Anyway, thanks again for your help. \$\endgroup\$ Commented Jan 16, 2014 at 20:34
  • \$\begingroup\$ @user341493 - Thank you for the fun question, and for considering my answer. \$\endgroup\$ Commented Jan 16, 2014 at 20:35
2
\$\begingroup\$

Expanding on part of Wayne's post, but taking a more declarative approach which avoids the case statement and makes the high level logic more visible, I came up with this:

VOWEL = "[aeiou]" def translate_word(word) restore_capitalization = (word =~ /^[A-Z]/) ? :capitalize : :to_s ret = word.downcase ret = (ret =~ /^#{VOWEL}/i) ? vowel_translator(ret) : consonant_translator(ret) ret.send(restore_capitalization) end def vowel_translator(word) suffixes = [ {re: /#{VOWEL}$/, suffix: 'yay'}, {re: /y$/, suffix: 'nay'}, {re: /./, suffix: 'ay'} ] correct_suffix = suffixes.find {|h| word =~ h[:re]}[:suffix] word + correct_suffix end def consonant_translator(word) consonants = word.slice!(/^[^#{VOWEL}]*/) word + consonants + "ay" end
\$\endgroup\$
4
  • \$\begingroup\$ An interesting approach! I would consider word = word.downcase; when practical, a function should either have side effects (such as modifying the argument) and return no value, or have no side effects and return a value. \$\endgroup\$ Commented Jan 22, 2014 at 16:25
  • \$\begingroup\$ Thanks Wayne. Isn't word.downcase! an example of having side effects (it mutates word) and returning no value? Of course, technically all ruby methods return some value, but here the value isn't used, so as far as the client code is concerned it is returning no value. Although for clarity I am tempted to rewrite the first line of the method as ret = word.downcase, leave the 2nd line as is, and then replace all occurences of word in the last 2 lines with ret. This breaks the method up into an "intro" section and a "body", per Avdi Grimm, a technique I like. \$\endgroup\$ Commented Jan 22, 2014 at 18:28
  • \$\begingroup\$ I meant #translate_word, which is currently both returning a value and modifying its argument. @Avdi Grimm is smart-do you have a link to where he suggested that technique? \$\endgroup\$ Commented Jan 22, 2014 at 18:32
  • 1
    \$\begingroup\$ Ah, yes good point. My other suggestion would answer that critique as well. I think I'll go ahead and make the edit. I believe that technique is a big theme of "Confident Ruby", and I think also discussed in this video. \$\endgroup\$ Commented Jan 22, 2014 at 18:48
1
\$\begingroup\$

Here is another way to write the translate_word method. I decided to subclass String, thinking it might improve readability. 

class Vowel < String; end class Consonant < String; end class Y < Consonant; end YAY = %w[y a y] NAY = %w[n a y] AY = %w[a y] def assign_chars_to_classes(word) raise ArgumentError, "Choked on #{word}" unless word =~ /^[a-z]*$/i word.downcase.chars.each_with_object([]) do |c,a| a << if "aeiou".include?(c) Vowel.new(c) elsif c == 'y' Y.new(c) else Consonant.new(c) end end end def translate_word(word) return nil if word.empty? w = assign_chars_to_classes(word) case w.first when Vowel w += case w.last when Vowel YAY when Y NAY else # Consonant AY end else # begins with a consonant while w.first === Consonant w.rotate! end if w.find {|c| c === Vowel} w += AY end w.first.capitalize! if word[0] == word[0].capitalize w.join end p translate_word('') # => nil p translate_word('ask') # => 'askyay' p translate_word('apple') # => 'appleyay' p translate_word('any') # => 'anynay' p translate_word('d') # => 'day' p translate_word('dog') # => 'ogday' p translate_word('Phony') # => 'Onyphay' p translate_word('zzzzi') # => 'izzzzay' p translate_word('cat3') # => 'Choked on cat3 (ArgumentError)'

Consider the word 'Phony'. The first step is

w = assign_chars_to_classes('Phony') # => ["p", "h", "o", "n", "y"]

where

w.map(&:class) # => [Consonant, Consonant, Vowel, Consonant, Y]

As w.first => "p" is of class Consonant (recall case uses ===), we see if w contains any vowels:

if w.find {|c| c === Vowel} # => "o"

As w contains at least one vowel, we move the leading consonants ("p" and "h") to the end:

while w.first === Consonant w.rotate! end # => ["o", "n", "y", "p", "h"]

capitalize the first element of w if the word is capitalized:

w.first.capitalize! if word[0] == word[0].capitalize # w => ["O", "n", "y", "p", "h", "a", "y"]

and join the elements of w:

w.join # => "Onyphay"

Edit: I initially had

w = w.chunk {|c| c.is_a? Consonant}.map(&:last).rotate.flatten + AY

in place of

while w.first === Consonant w.rotate! end if w.find {|c| c === Vowel}

but prefer what I have now.

\$\endgroup\$
2
  • \$\begingroup\$ Here case w.first when Vowel w += I think you forgot to increment w..as I am seeing it as w += only.. \$\endgroup\$ Commented Jan 13, 2014 at 4:56
  • \$\begingroup\$ @Arup, I'm appending what is returned by case to w. I just formatted it this way so case w.last would stand alone (i.e., line-continuation character `\` not required). \$\endgroup\$ Commented Jan 13, 2014 at 23:18

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.