Menu-based Caesar Cipher with encryption and decryption

Question

I am about to start a very large project in Ruby. To get myself in the Ruby mindset I have been doing all of my homework and other side work in Ruby. I am hoping that some of you who know Ruby very well can look at this small program I have written and point out areas that aren't written in a Ruby-like fashion. Any and all tips and criticisms would be appreciated.

This program is a menu-based Caesar Cipher with encryption and decryption.

KEY = 3 
$caesar_map = {}

#initializes the caesar_map and reads input from the user
#starting point of the program
def init()
  #create empty hashes
  plain_alphabet  = {}
  cipher_alphabet = {}

  #fill in the alphabet hashes with the letter and its position
  #in the english alphabet
  "a".upto("z") {|x| plain_alphabet[x]  = ((x[0] - "a"[0]) % 26) }
  "A".upto("Z") {|x| cipher_alphabet[x] = ((x[0] - "A"[0]) % 26) }

  #fill in the caesar_map, maps every letter KEY positions
  #to the right in the alphabet
  plain_alphabet.each do |k,v|
    $caesar_map[k] = cipher_alphabet.index((v+KEY) % 26)
    #puts "key: #{k} mapped value: #{cipher_alphabet.index((v+3) % 26)}"
  end
  #special case, I want spaces to be maintained in the messages
  $caesar_map[" "] = " "

  #print menu
  choice = -1
  while(choice != 3)
    puts "What would you like to do?"
    puts "\n1. Encrypt a message"
    puts "\n2. Decrypt a message"
    puts "\n3. Exit\n"

    choice = Integer(gets.chomp)

    if(choice == 1)
      puts "Please enter a message you would like to Encrypt\n"
      message = gets.chomp
      encrypt(message)
    elsif(choice == 2)
      puts "Please enter a message you would like to Decrypt\n"
      message = gets.chomp
      decrypt(message)
    elsif(choice == 3)
      abort("Exiting...")
    else
      puts "You didn't enter a valid choice. Enter a number between 1..3\n"
    end
  end
end

#encrypts a plaintext message using the caesar cipher
def encrypt(plaintext)
  #make sure the message is in lowercase
  plaintext.downcase!
  ciphertext = ""

  #fill in the ciphertext with mapping information in the caesar_map
  plaintext.split("").each do |c|
    ciphertext += $caesar_map[c]
  end

  #print the encrypted ciphertext
    puts "\n plaintext: #{plaintext}\nciphertext: #{ciphertext}\n\n"
end

#decrypts a ciphertext message using the caesar cipher
def decrypt(ciphertext)
  #make sure the ciphertext is in uppercase
  ciphertext.upcase!
  plaintext = ""

  #fill in the plaintext with mapping information in the caesar_map
  ciphertext.split("").each do |c|
    plaintext += $caesar_map.index c
  end

  #print the decrypted plaintext
  puts "\n ciphertext: #{ciphertext}\n plaintext: #{plaintext}\n\n"
end

#start the program
init()

Answer 1

Some points about making your code look more like idiomatic Ruby:

• Use nil.

  choice = -1
  

  should be

  choice = nil
  

• Don't wrap your conditional in brackets:

  while(choice != 3)
  if(choice == 1)
  

  should be

  while choice != 3
  if choice == 1
  

• Instead of initializing a variable to an empty array, use map to convert one array directly into the new form and use that to initialize your variable:

  Instead of this: Hashes != arrays, sorry. The principal still applies, even if this example isn't equivalent:

   #create empty hashes
   plain_alphabet  = {}
   cipher_alphabet = {} 
  
  "a".upto("z") {|x| plain_alphabet[x]  = ((x[0] - "a"[0]) % 26) }
  "A".upto("Z") {|x| cipher_alphabet[x] = ((x[0] - "A"[0]) % 26) }
  

  do this:

  plain_alphabet  = ("a".."z").map { |x| (x[0] - "a"[0]) % 26 }
  cipher_alphabet = ("A".."Z").map { |x| (x[0] - "A"[0]) % 26 }
  

  And instead of this:

  ciphertext = ""
  
  #fill in the ciphertext with mapping information in the caesar_map
  plaintext.split("").each do |c|
    ciphertext += $caesar_map[c]
  end
  

  do this:

  ciphertext = plaintext.split("").map { |c| $ceasar_map[c] }.join
  

• Use case when appropriate; rewrite your if/elsif/elsif/else as:

  case choice
  when 1:
    # ...
  when 2:
    # ...         
  when 3:
    # ...
  else
    # ...
  end
  

Answer 2

I'd initialize with something closer to this (although I might inline the two arrays):

plain_alphabet = ('a'..'z').to_a
cipher_alphabet = ('A'..'Z').to_a.rotate KEY
$caesar_map = Hash[plain_alphabet.zip cipher_alphabet]

The en/decryption (which could likely be handled by the built-in tr method) might look more like:

plaintext.split("").inject("") {|s, c| s << $caesar_map[c]}

I'd put the whole thing in a class, too; skip the global.

Answer 3

KEY = 3 
$caesar_map = {}

In Ruby, like most languages, global variables are often a sign of bad design. In this making $caesar_map global makes it difficult to use your caeser cipher with different keys (though your program isn't layed out to support that anyway (and you even made KEY constant)) because in order to do that you'd have to override the global variable each time you want to use a different key.

I would recommend to create a class which takes the key as a parameter and then creates the map as a parameter. So you could create multiple instances with different keys and then use their encode and decode methods without them affecting each other.

Except that pre-calculating the mappings between characters seems completely unnecessary to me, so that's not even necesarry. You could just write encode and decode so that they take the key as a parameter and just do the translation without a map. This way you could call the methods without having to initialize any global data structures or objects. And you could just implement decode by calling encode with a negative key, which greatly simplifies your implementation and gets rid of code duplication.

---

Further your encode and decode methods should return string rather than printing their results. It is a good design practice to seperate the places that do IO from the places that calculate stuff. That makes it easier to reuse your logic-methods.

Also you shouldn't call your init method init. That name suggests that all it does is initializing somethign (say $caesar_map), but in fact it does everything. A more appropriate name might be main. Or you could just get rid of the method altogether.

---

"a".upto("z") {|x| plain_alphabet[x]  = ((x[0] - "a"[0]) % 26) }
"A".upto("Z") {|x| cipher_alphabet[x] = ((x[0] - "A"[0]) % 26) }

As I already said, you don't really need to precalculate anything, so this becomes superfluous, but here's some comments for when you write similar code in the future:

"a"[0] returns "a" in ruby 1.9, so you should use .bytes[0] instead of just [0] for 1.9 compatibility. Also the % 26 is completely unnecessary here as the number will already be between 0 and 26.

And as has already been pointed out by Dave, using map or other higher-order functions for things like this is a lot more idiomatic. When you want to build hashes like this, you might also take advantage of the fact that Hash.new takes a block.

---

$caesar_map[" "] = " "

You special case " ", but nothing else. If the string contains anything except letters or spaces, your method will break.

---

choice = Integer(gets.chomp)

This will throw an exception when the user enters a non-number. You might perhaps catch that exception and give the user something a bit more friendly than an exception-message.

---

#make sure the message is in lowercase
plaintext.downcase!

Returning the result with the same case as the original had (instead of lower casing the input string and then using upper case letters in the translation) seems much preferable.

---

plaintext.split("").each do |c|
  ciphertext += $caesar_map[c]
end

Building strings up imperatively like this is often not very idiomatic ruby. In this case I'd use gsub:

plaintext.gsub(/[a-zA-Z]/) do |c|
  $caesar_map[c]
end

Since this only replaces letters and leaves everything else as-is, this also gets rid of your problem of not supporting non-letters (and gets rid of the need to special-case " " in the map).

If you follow my advice from earlier and get rid of the global map, you'll have to adjust the above appropriately, of course.

---

$caesar_map.index c

Note that using Hash#index is O(n), so this is terribly inefficient (same goes for when you used it earlier in init). If you follow my advice and don't use a map, this will be much more inefficient.

---

In summary, here's how I'd implement the en- and decoding:

# Shifts a given letter by key places in the alphabet, wrapping around after "z",
# keeping the case in tact.
# Do not call with anything that's not a letter between a and z (or A and Z)
def shift_letter(letter, key)
  case letter
  when "a".."z"
    base = "a".bytes[0]
  when "A".."Z"
    base = "A".bytes[0]
  end

  # Set letter_index to the index of the given letter (i.e. 0 for A or a, 1 for b etc.)
  letter_index = letter.bytes[0] - base

  # Calculate the index of the mapped letter
  mapped_index = (letter_index + key) % 26

  # Convert index back to letter
  (base + mapped_index).chr
end

# Encrypts a plaintext message using the Caesar Cipher
def encode(plaintext, key)
  # Replace each letter in the string with the result of shifting
  # that letter by key places
  plaintext.gsub(/[a-zA-Z]/) do |c|
    shift_letter(c, key)
  end
end

# Decrypts a ciphertext message using the Caesar Cipher
def decode(ciphertext, key)
  encode(ciphertext, -key)
end