4
\$\begingroup\$

I just finished the pig latin translator for The Odin Project and I would appreciate some honest feedback for my code. The code is intended to be "test-driven learning", and therefore emulates test driven development.

Here is the prompt:

Pig Latin is a made-up children's language that's intended to be confusing. It obeys a few simple rules (below) but when it's spoken quickly it's really difficult for non-children (and non-native speakers) to understand.

Rule 1: If a word begins with a vowel sound, add an "ay" sound to the end of the word.

Rule 2: If a word begins with a consonant sound, move it to the end of the word, and then add an "ay" sound to the end of the word.

(There are a few more rules for edge cases, and there are regional variants too, but that should be enough to understand the tests.)

Here is the test spec:

require "pig_latin"

describe "#translate" do

  it "translates a word beginning with a vowel" do
    s = translate("apple")
    expect(s).to eq("appleay")
  end

  it "translates a word beginning with a consonant" do
    s = translate("banana")
    expect(s).to eq("ananabay")
  end

  it "translates a word beginning with two consonants" do
    s = translate("cherry")
    expect(s).to eq("errychay")
  end

  it "translates two words" do
    s = translate("eat pie")
    expect(s).to eq("eatay iepay")
  end

  it "translates a word beginning with three consonants" do
    expect(translate("three")).to eq("eethray")
  end

  it "counts 'sch' as a single phoneme" do
    s = translate("school")
    expect(s).to eq("oolschay")
  end

  it "counts 'qu' as a single phoneme" do
    s = translate("quiet")
    expect(s).to eq("ietquay")
  end

  it "counts 'qu' as a consonant even when it's preceded by a consonant" do
    s = translate("square")
    expect(s).to eq("aresquay")
  end

  it "translates many words" do
    s = translate("the quick brown fox")
    expect(s).to eq("ethay ickquay ownbray oxfay")
  end

  # Test-driving bonus:
  # * write a test asserting that capitalized words are still capitalized (but with a different initial capital letter, of course)
  # * retain the punctuation from the original phrase

end

Finally, here is my code:

def translate(phrase) #translates individual words or multiple words into pig latin
  new_phrase = []
  if phrase.include? " "
    new_phrase = phrase.split(" ").map do |word|
      rearrange(word)
    end
    new_phrase = new_phrase.join(" ")
  else
    new_phrase = rearrange(phrase)
  end
  new_phrase
end

def rearrange(word) #rearranges individual words into pig latin
  vowels = ["a", "e", "i", "o", "u", "y"]
  new_word = word
  word.each_char do |char|
    if char == "u" && new_word[-1] == "q"
      new_word << "u"
      new_word.slice!(0)
      next
    elsif vowels.include? char
      new_word << "ay"
      break
    else
      new_word << char
      new_word.slice!(0)
      next
    end
  end
  new_word
end
\$\endgroup\$
1
\$\begingroup\$

Some notes:

  • expect(translate("three")).to eq("eethray"). Be consistent, use always a local variable or don't. I think this way looks pretty cool.

  • Isn't the code inside a module/class?

  • translate. This methods is unnecessarily verbose. You don't need to check if there is a space or not, since the algorithm works just the same if there is not.

  • Those two nexts are redundant.

  • rearrange: This method would look so much nicer if you don't use imperative style.

  • If a regular expression can express the rules, you should consider using it. It will be easier to read and much more compact.

  • Side note: The rules are pretty simple. If they ever get more complex, a manual processing will be undecipherable and regexps will come short. Then you'll need something more sophisticated, like a parser (i.e. grammy).

I'd write:

def translate(phrase)
  phrase.split(" ").map { |word| rearrange(word) }.join(" ") 
end

def rearrange(word)
  match = word.match(/^((?:qu|[bcdfghjklmnpqrstvwxz])*)(.*)$/)
  match ? match[2] + match[1] + "ay" : word
end

If you have trouble reading that regular expressions, consider using multi-line regexps with named captures:

def rearrange(word)
  match = word.match(%r{
    ^
    (?<leading_consonants>(?:qu|[bcdfghjklmnpqrstvwxz])*) 
    (?<rest>.*)
    $)
  match ? match[:rest] + match[:leading_consonants] + "ay" : word
end
\$\endgroup\$
  • \$\begingroup\$ Thank you! I kinda figured that the methods were too verbose, I appreciate the feedback. \$\endgroup\$ – Adrian DeRose Nov 12 '16 at 1:42

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.