Pig Latin Translator

Question

Could someone review the following code?

class PigLatin

  attr_accessor :phrase
  ending = "ay"

  def initialize the_phrase
    @phrase = the_phrase
  end

  def translate
    translated_phrase = String.new
    words = @phrase.split
    words.each do |word|
      puts word
      if vowel_is_first word
        translated_phrase +=  translate_with_vowel(word)
      else
        translated_phrase += translate_with_consonant(word)
      end
    end

    clean_up translated_phrase
  end

  def vowel_is_first test_word
    true if test_word[0] =~ /aeiou/ #will this make false since 0??
    false
  end

  def translate_with_vowel word
    word + "-" + ending + " "
  end

  def translate_with_consonant word
    return word + "-way " if word.size <= 2
    split_location = word =~ /a|e|i|o|u/
    word_array_length = word.size - 1

    first_segment = word[split_location,word_array_length]
    second_segment = "-" + word[0,split_location-1] + word[split_location-1]  + "ay" + " "

    first_segment + second_segment
  end

  def clean_up to_be_cleaned
    to_be_cleaned = to_be_cleaned[0..to_be_cleaned.size-2]
    to_be_cleaned.capitalize
  end
end

I'm sure it could use some work as I'm just getting going in Ruby, so anything and everything would be helpful. The clean-up function, I think, could especially use work. It gets the job done, but w/o it there is always a space at the end, and str.strip wasn't getting the job done.

Answer 1

First of all there are two mistakes in the code:

1. ending = "ay" is a local variable only visible directly in the class body, not inside of the defs. For this reason translate_with_vowel will throw a NameError when it is called. You should make it a constant, so it'll be visible everywhere.
2. The reason that you did not notice the first mistake is that translate_with_vowel is never called because vowel_is_first will always return false. You should use if ... else ... end instead of the inline if. The reason that it doesn't work the way you wrote it is that since true if test_word[0] =~ /aeiou/ is not the last expression in the method, so its return value simply gets discarded and it doesn't have any side-effects.

---

From a design perspective, I'm a bit doubtful that having a PigLatin object for each string you want to translate is really a good idea. Each PigLatin object has one instance variable, which is set in initialize and used by exactly one method (translate). All other methods only depend on their arguments and not on the state of the object. So from a programmer's point of view, having the string encapsulated in an object buys you nothing over taking it as an argument to translate.

From the user's point of view it also makes the API more cumbersome than it needs to be. There's no reason to prefer PigLatin.new(string).translate over PigLatin.translate(string) if there's no reason why I'd ever call more than one method on an object, there really is no reason for the object to exist at all.

If the translation was parametrized by some configuration options, it would make sense to have a class which is instantiated with the options and a translate method which takes the string, like this:

pig_latin = Translator.new(:input => :english, :output => :pig_latin)
string1 = pig_latin.translate("Hello World")
string2 = pig_latin.translate("Goodbye World")

However since in this case there are no options, I'd recommend using a PigLatin module and defining all the methods as module_functions. This way the usage would be PigLatin.translate("Hello World") or include PigLatin; translate("Hello World").

Also since there's no reason why a user should ever call any of your methods except translate, the other methods should probably be private.

---

  def translate
    translated_phrase = String.new
    words = @phrase.split
    words.each do |word|
      puts word
      if vowel_is_first word
        translated_phrase +=  translate_with_vowel(word)
      else
        translated_phrase += translate_with_consonant(word)
      end
    end

    clean_up translated_phrase
  end

I assume the puts is just an old debug statement you forgot to remove. If it isn't, be advised that mixing IO with logic is usually bad design. If it is, a little tip: If you use p instead of puts for debug statements, you will get output which is more useful because you can see whether the object has the type you expect and, in case of strings, whether it contains any unprintable meta characters.

String.new is just a more verbose way to say "", so you should use that instead. However instead of appending to an empty string in an each loop, I'd rather use map and join. join will also take care of inserting spaces between the elements for you (if you tell it to, that is), so you don't need to add those spaces yourself in the translate_ methods and you don't have to remove the space at the end.

So I'd write translate like this:

 def translate(string)
    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(" ")
    translated_phrase.capitalize
  end

You can also define another helper method translate_word which contains the above if statement and then simplify the above to translated_words = words.map(&:translate_word).

---

  def translate_with_consonant word
    return word + "-way " if word.size <= 2
    split_location = word =~ /a|e|i|o|u/
    word_array_length = word.size - 1

    first_segment = word[split_location,word_array_length]
    second_segment = "-" + word[0,split_location-1] + word[split_location-1]  + "ay" + " "

    first_segment + second_segment
  end

The whole split_location is overly complicated. Just use String#split (which accepts an integer as a second argument, which you can use to tell it that you only want to split the string into two parts). You may also want to use string interpolation instead of concatenation. And as I said earlier, you can get rid of the + " " as that's now handled by join.

I'd write it like this:

  def translate_with_consonant word
    return word + "-way " if word.size <= 2
    second_segment, first_segement = word.split(/a|e|i|o|u/, 2)

    "#{ first_segment }-#{ second_segment }ay"
  end