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\$
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\$
  • \$\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\$ – user341493 Jan 16 '14 at 20:34
  • \$\begingroup\$ @user341493 - Thank you for the fun question, and for considering my answer. \$\endgroup\$ – Wayne Conrad Jan 16 '14 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\$
  • \$\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\$ – Wayne Conrad Jan 22 '14 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\$ – Jonah Jan 22 '14 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\$ – Wayne Conrad Jan 22 '14 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\$ – Jonah Jan 22 '14 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\$
  • \$\begingroup\$ Here case w.first when Vowel w += I think you forgot to increment w..as I am seeing it as w += only.. \$\endgroup\$ – Arup Rakshit Jan 13 '14 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\$ – Cary Swoveland Jan 13 '14 at 23:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.