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

English Translator - follow up

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

Problem

Awhile ago I posted a Translator app that I have been in the process of making. I edited some features in it and I've decided to post it again.

My questions:

  • Is there anything I should add to this?



  • Is there any better (more easily readable) syntax for what I'm trying to do?



  • Say I wanted to make a GUI for this app. What language goes well with Ruby?



Here's the source code of the app:

```
require 'yaml'
require 'base64'
require 'digest'

def welcome_list
puts """
Welcome to Translation Center! Below you will find a list of available(so far) features, more feautures are being added as we speak! Have fun and enjoy!

As of now Translation Center is able to translate from:

1: Russian
2: Spanish
3: Arabic
4: Pashto
5: French

Translation Center is also able to translate into:

6: Base64
7: Binary
8: MD5

Please choose from the list above by typing in the number in front of the language:
"""
input = gets.chomp!
case input.to_i
when 1
translate
when 2
translate
when 3
translate
when 4
translate
when 5
translate
when 6
base64
when 7
binary
when 8
md5
else
puts "Invalid input, redirecting..."
return
end
end

def translate
translations = YAML.load_file 'words.yml'
puts "Enter word or phrase to be translated to English, press 'Q' to quit:"
input = gets.chomp
if translations[input]
puts "The translation of '#{input}' to English is: '#{translations[input]}'"
elsif input =~ /q/i
list
else
puts "Invalid word or phrase redirecting..."
translate
end
end

def binary
puts "Enter your text you wanted translated to binary:"
text = gets.chomp
puts "#{text} in binary is: #{text.unpack('b*')}"
binary if restart
end

def base64
puts "Enter the text you want translated to Base64:"
input = gets.chomp!.to_s
new_

Solution

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

Code Snippets

elsif input =~ /q/i
  list
russian:
    А: A
    В: V
russian:
А: A
В: V
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
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

Context

StackExchange Code Review Q#109543, answer score: 4

Revisions (0)

No revisions yet.