A few dozen teensy little things before I get to the meat of my review:
"""
does not a multiline string make. It makes an empty string, a multiline string, and another empty string, all joined with newlines. Replace that with a heredoc -- this blog post goes into a good bit of detail about them.
- I can't find a source for it, but console windows are typically 80 characters wide. Anything more and you get wrapping, which may look fine on *nix but on Windows it looks awful, because it's character-based wrapping, not word-based.
- You made a bunch of grammar errors. I fixed 'em. Of course, most of the text could be reworked -- from the menu, you'd never guess that you can't translate to Russian, Spanish, Arabic, Pashto, or French, and you can't translate from base 64, binary, or MD5 hash. Well, maybe the last, but the rest it's not clear.
- In a
case
statement, you can see if a number is within a range by doing when lower_bound..upper_bound
. Standard range syntax applies.
- You load the
YAML
file every time someone wants to translate something. Load it once, then either make it a $global
variable or pass it in through translate
's arguments.
- When you say "redirecting", you probably mean "please try again", since there isn't a program in the world that "redirects" to the same menu when you mess up the input. You also don't want to use recursion to do that, since at some apparently random point they'll get a
SystemStackError
for recursing too deeply. There's a reason that Ruby has iterative loops.
- On that note, why is it that when you enter invalid input anywhere else, you can try again, but in
welcome_list
, the program says the same thing but then exits? Just remove the text and have it end, or drop a loop in like everything else.
- In
md5
, why is the hash called input_string
? It's very obviously not what they inputted. That should really be called something like hash
or result
.
- Some of your English is inconsistent -- personally, I like it all to look the same, because it gives it a more clean feel if you see
'Enter the text you want translated to [language]:'
and 'abc' translated to [language] is: 'def'
every time, instead of different text for each "language".
What on Earth is with this bit:
elsif input =~ /q/i
list
Why does anything containing Q -- no, not something that's just that letter, read the documentation -- run a method, list
, that doesn't exist? See, little teensy break-your-whole-program-no-matter-what bugs like that are why you have to test your code. I have no idea what you're trying to do here (i.e. go back to the welcome list or exit the program) so I assumed you wanted to exit the program. It's easier on me.
- You use
gets.chomp!.to_s
, which makes no sense, because chomp!
already returns a string, and the bang version (chomp!
instead of chomp
) affects nothing in this case. You use it right elsewhere, so I'm confused.
NOTE: In refactoring the whole program according to my description below, some of these things become invalid. However, they're still worth mentioning.
Right, on to the fun stuff. Namely, why your program is absolutely terrible in one teeny tiny edge case that you should have seen a mile away: Two languages that have the same word will seriously screw each other up. See, you divide your YAML file into human-readable pieces by doing a little header with the language, but YAML doesn't see it like that. What it sees is a key, language_name
, that's mapped to nothing at all. If you wanna say "These are all the words in Russian" and have YAML associate "russian" with those words, you have to indent1. For example:
russian:
А: A
В: V
instead of
russian:
А: A
В: V
Now, fixing this is gonna cause a few problems, because you were relying on your bad way of doing things. First off, translate
is depending on everything being in the bottom level. Instead, what you're gonna have to do is change it like this:
Add a parameter, all_words
, and translations = all_words[language]
before the first line.
Whew! That was an awful lot of effort. Now, that just broke all your code, because you're calling translate
with only one argument, up above. What you're gonna wanna do is one of two things:
- Hardcode your list into the
case
, sort of like it was before. I recommend against this.
- Make it all super easy to change by having it sense what languages are available, update the list automatically, and put the appropriate options in the
case
.
Since the way I did it is pretty complicated, and involved an annoying amount of refactoring, I'm not gonna describe it in great detail. See the code if you want to know how. It's not that hard, just a different way of thinking.
There are a few remaining problems that I'm not going to fix, because I'm a lazy son of a you should practice. They are:
- You mix input with calculation -- the calculation should ideally be separated from the input as much as is feasible. That is, if you're using
puts
and gets
in the same method as the one where you perform a calculation, separate them. Note that this isn't always true -- if the calculation is solely for the purpose of dynamic text, for example, you shouldn't separate that out. Probably. It depends.
- You made it almost impossible to easily loop the whole program, instead of just bits, because you focused on using recursion.
Note: As a side effect of this, I couldn't easily figure out how to make restart
work without recursion, so I left it alone.
- In
translate
, if the word happens to be Q
or q
(which is unlikely but possible), you won't give the translation. You need to listen for CTRL+C (i.e. InterruptError
if memory serves) and quit on that, then say that you're quitting on that.
- If they enter the text in anything but all lowercase, it doesn't give them the translation, unless you put the uppercase versions in the YAML manually.
binary
prints out the result as ["100001101000011010000110"]
instead of 100001101000011010000110
(for the string aaa
).
(mostly) Fixed code:
require 'yaml'
require 'base64'
require 'digest'
def welcome_list
word_bank = YAML.load_file 'words.yml'
languages = word_bank.keys
non_language_options = 3
i = non_language_options
puts <<-END.gsub(/^\s*>/, '')
>Welcome to Translation Center! Below you will find a list of as yet available features.
>More features are being added as we speak! Have fun and enjoy!
>
>Translation Center can translate to:
>
>1: Base64
>2: Binary
>3: MD5
>
>Translation Center can translate from:
>
>#{languages.map { |item| "#{i+=1}: #{item}" }.join "\n"}
>
>Please choose from the list above by typing in the number in front of the option:
END
input = gets.chomp
case choice = input.to_i
when 1
base64
when 2
binary
when 3
md5
when (non_language_options+1)..(languages.length+non_language_options+1)
translate(word_bank, languages[choice-non_language_options-1])
else
puts 'Invalid input.'
return
end
end
def translate(all_words, language)
translations = all_words[language]
while true
puts "Enter word or phrase to be translated from #{language} to English, press 'Q' to quit:"
input = gets.chomp
if translations[input]
puts "The translation of '#{input}' to English is: '#{translations[input]}'"
next
elsif input.upcase == 'Q'
exit
else
puts 'Invalid word or phrase. Please try again.'
next
end
break
end
end
def binary
while true
puts 'Enter the text you want translated to binary:'
text = gets.chomp
puts "#{text} in binary is: #{text.unpack('b*')}"
break unless restart
end
end
def base64
while true
puts 'Enter the text you want translated to Base64:'
input = gets.chomp
new_string = "#{input}"
puts "'#{input}' in Base64 is: #{Base64.encode64(new_string)}"
break unless restart
end
end
def md5
while true
puts 'Enter the text you want translated to MD5:'
input = gets.chomp
result = Digest::MD5.new
result.update(input)
puts "'#{input}' translated to MD5 is: '#{result}'"
break unless restart
end
end
def restart
puts <<-END.gsub(/^\s*>/, '')
>Would you like to:
>1. Translate another
>2. Go back to the main menu
>3. Quit
END
input = gets.chomp!
if input == '1'
true
elsif input == '2'
puts 'Redirecting to menu...'
welcome_list
else
puts 'Exiting...'
exit
end
end
welcome_list
Fixed words.yml
:
spanish:
a: to
abajo: down
arabic:
salam: Hi
ana min: I'm from...
russian:
А: A
В: V
french:
oui: Yes
non: No
pashto:
khe chare: Hello
japanese:
yaa: Hi
Yaa: Hi
Konbanw: Good evening
As for a GUI, I suggest Shoes!, because it's conveniently already in Ruby.
1: Little teensy things like this are why you always, always, always, always read the documentation for everything you're using. ALWAYS.