HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Pig Latin Translator - follow-up

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
translatorlatinfollowpig

Problem

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

Solution

Some points in addition to what checkorbored pointed out:

-
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.

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

-
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.

-
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.

Code Snippets

return foo if bar
return baz
if bar
  foo
else
  baz
end
def initialize (options = {:dash => true})
  @dash = options[:dash]
end
def initialize (options = {})
  options = {:dash => true}.merge(options)
  @dash = options[:dash]
end

Context

StackExchange Code Review Q#2353, answer score: 4

Revisions (0)

No revisions yet.