First of all there are two mistakes in the code:
ending = "ay"
is a local variable only visible directly in the class body, not inside of the def
s. 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.
- 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_function
s. 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
0
is "truthy" in ruby, so=~
returning0
will not lead to problems in anif
. \$\endgroup\$