# Caesar Cipher, done in Ruby

It's an assignment from the Odin Project.

Here's my implementation:

#!/usr/bin/env ruby

def caesar_cipher(str, shift)
low_alpha = ("a".."z").to_a
high_alpha = ("A".."Z").to_a
length_alpha = low_alpha.length

chars = str.split("")
ciphered = ""

chars.each do |char|
if low_alpha.include? char
i = low_alpha.index(char)
shifted = (i + shift) % length_alpha

ciphered << low_alpha[shifted]
elsif high_alpha.include? char
i = high_alpha.index(char)
shifted = (i + shift) % length_alpha

ciphered << high_alpha[shifted]
else
ciphered << char
end
end

ciphered
end

puts caesar_cipher("What a string!", 5) # Bmfy f xywnsl!
puts caesar_cipher("Abc", 5) # Fgh
puts caesar_cipher("Xyz", 3) # Abc
puts caesar_cipher("Test", 1) # Uftu
puts caesar_cipher("Zoo", 10) # Jyy


What points could be improved? What would you have done differently and why?

• It could be simplified if you used .ord (example 'a'.ord shows 97), it shows the numerical value of the character, so instead of checking if the character is in an array you could check if it is present in a range making the comparison more efficient. – Miguel Avila Jun 21 at 17:48

# Indentation

The standard indentation style in Ruby is two spaces, not four.

# Single-quoted strings

If you don't use string interpolation, it is helpful if you use single quotes for your strings. That way, it is immediately obvious that no string interpolation is taking place.

# Frozen string literals

Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:

# frozen_string_literal: true


It is generally preferred to add this comment to all your files. In your case, you are only actually mutating one of the many strings in your code, the one assigned to ciphered.

You can make only this string mutable by either not using a literal to initialize it:

ciphered = String.new


Or by using the unary prefix String#+@ operator for strings, which turns a frozen string into a mutable string:

ciphered = +''


# Linting

You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.

Rubocop was able to detect all of the style violations I pointed out, and also was able to autocorrect all of them. Note, however, that automatically adding the magic comment broke the code originally, because Rubocop does not automatically detect which strings are mutated. So, I had to add the + manually afterwards. That's where good tests come in handy!

# Inconsistent use of parentheses

Sometimes, you use parentheses around arguments, and sometimes you don't. The general rule is to always use parentheses around arguments if you have arguments, and leave out the parentheses only for "procedure-like" methods such as puts, attr_reader, require, private, etc.

For example, you are using parentheses for split and index but not for include?.

# Constants

There is no need to re-compute low_alpha, high_alpha, and length_alpha every time you call the method. Their values will always be the same. Therefore, it makes sense to turn them into constants and initialize them at the beginning of the file:

LOW_ALPHA = ('a'..'z').to_a
HIGH_ALPHA = ('A'..'Z').to_a
LENGTH_ALPHA = LOW_ALPHA.length


# Object#freeze

Object#freeze is a method that allows you to freeze an object. A frozen object will no longer allow itself to be modified. It is good practice in general to freeze objects that you don't intend to modify, both as a signal to the reader that this object will not be modified, and as a safety net, in case you ever accidentally try to modify it regardless.

We already made all but one of our strings frozen, so let's do that with the arrays as well:

LOW_ALPHA = ('a'..'z').to_a.freeze
HIGH_ALPHA = ('A'..'Z').to_a.freeze
LENGTH_ALPHA = LOW_ALPHA.length


Numbers are immutable anyway, no need to freeze them.

# length vs. size

Many Ruby collections have both length and size methods, but some have only one. In general, IFF a collection has a size method, then that method is guaranteed to be "efficient" (usually constant time), whereas length may or may not be efficient (linear time for iterating through the collection and counting all the elements), depending on the collection.

In your case, you are using an array, for which both are constant time, but if you want to guarantee efficiency, then it is better to explicitly use size instead.

# String#chars

Instead of using String#split, you can use String#chars to create an array of characters:

chars = str.chars


# String#each_char

Actually, you don't need the array of characters at all. Instead, you can use the String#each_char iterator directly:

str.each_char do |char|


# The conditional expression is … an expression

In Ruby, the conditional expression if / else is an expression, not a statement. (In fact, everything in Ruby is an expression, there are no statements.) Therefore, the conditional expression evaluates to a value, it evaluates to the value of the branch that was taken.

This means you can remove the duplicated ciphered << X from each branch, and instead pull it out of the conditional expression:

ciphered << if LOW_ALPHA.include?(char)
i = LOW_ALPHA.index(char)
shifted = (i + shift) % LENGTH_ALPHA

LOW_ALPHA[shifted]
elsif HIGH_ALPHA.include?(char)
i = HIGH_ALPHA.index(char)
shifted = (i + shift) % LENGTH_ALPHA

HIGH_ALPHA[shifted]
else
char
end


# Code duplication

Speaking of duplicated code: Your then branch and your elsif branch are virtually identical. We can simplify them by extracting the duplicated code into a method:

def encrypt_letter(char, alphabet, shift)
i = alphabet.index(char)
shifted = (i + shift) % LENGTH_ALPHA

alphabet[shifted]
end


and then using this method in the two branches instead:

ciphered << if LOW_ALPHA.include?(char)
encrypt_letter(char, LOW_ALPHA, shift)
elsif HIGH_ALPHA.include?(char)
encrypt_letter(char, HIGH_ALPHA, shift)
else
char
end


# Higher-level iterators

Ruby has many powerful iteration methods in its collections library. Using each (or in this case each_char) directly is almost always sub-optimal. This particular pattern that you are using:

• Initialize an accumulator. (In this case the string assigned to ciphered.)
• Iterate over the collection and add to the accumulator.
• Return the accumulator.

Is known as a Fold, and is available in Ruby in two forms, Enumerable#each_with_object and Enumerable#inject. Using Enumerable#each_with_object, we can further simplify your code to:

def caesar_cipher(str, shift)
str.each_char.each_with_object(+'') do |char, ciphered|
ciphered << if LOW_ALPHA.include?(char)
encrypt_letter(char, LOW_ALPHA, shift)
elsif HIGH_ALPHA.include?(char)
encrypt_letter(char, HIGH_ALPHA, shift)
else
char
end
end
end


# The right higher-level iterator!

But actually, what you are doing here is simply transforming each element of the collection. You don't need something as powerful as a fold for that. This is a much simpler operation called Map, and it is also available in Ruby as Enumerable#map:

str.each_char.map do |char|
if LOW_ALPHA.include?(char)
encrypt_letter(char, LOW_ALPHA, shift)
elsif HIGH_ALPHA.include?(char)
encrypt_letter(char, HIGH_ALPHA, shift)
else
char
end
end.join


# Rubocop, revisited

I didn't mention this before, but in addition to the style violations I mentioned at the beginning, Rubocop was also complaining about the complexity and the length of the caesar_cipher method. At this point, Rubocop is actually happy with everything!

But we can do better.

# The Algorithm

What the Caesar Cipher is really doing, is shifting the alphabet. You have recognized this, as can be seen by your variable names (shift), but you are not really taking advantage of it.

What we can do, is take our alphabets, shift them, and then use them for a key-value mapping, i.e. a Hash:

Now, our entire code looks like this:

#!/usr/bin/env ruby
# frozen_string_literal: true

LOW_ALPHA = ('a'..'z').to_a.freeze
HIGH_ALPHA = ('A'..'Z').to_a.freeze

def caesar_cipher(str, shift)
low_encrypted = LOW_ALPHA.rotate(shift)
high_encrypted = HIGH_ALPHA.rotate(shift)

character_map =
Hash.new { |_, k| k }
.merge((LOW_ALPHA.zip(low_encrypted) + HIGH_ALPHA.zip(high_encrypted)).to_h)
.freeze

str.each_char.map(&character_map).join
end


Okay, there's a lot going on here. We are using Array#rotate to create the shifted character arrays:

LOW_ALPHA.rotate(3)
# => ["d", "e", "f", …, "a", "b", "c"]


Then we use Array#zip to create an array of pairs with the original character and the encrypted character:

LOW_ALPHA.zip(LOW_ALPHA.rotate(3))
# => [["a", "d"], ["b", "e"], ["c", "f"], …, ["x", "a"], ["y", "b"], ["z", "c"]]


With Array#+ we concatenate the two arrays together, and then call Array#to_h, which turns an arrays of pairs (two-element arrays) into a hash, which looks like this:

{
'a' => 'd',
'b' => 'e',
'c' => 'f',
# …
'x' => 'a',
'y' => 'b',
'z' => 'c',

'A' => 'D',
'B' => 'E',
'C' => 'F',
# …
'X' => 'A',
'Y' => 'B',
'Z' => 'C',
}


This contains our entire encryption scheme.

We have already created a hash with a default value that will simply return the key for any unknown key (so that, e.g. '!' maps to '!') and use Hash#merge to merge these two hashes into one.

Last, we call map as before, but now, as our transformation function, we simply pass the hash itself. For that, we use the & operator, which turns a Proc into a block. But wait, we don't have a Proc, we have a Hash? Indeed, but Hash implements Hash#to_proc, which converts the hash into a proc that is equivalent to using the hash's Hash#[] method.

# The power of strings

The String class in Ruby is really powerful as well. For example, it has the method String#tr which does the same thing as the POSIX tr utility, it translates characters in a string. This is really the right method to use for this job:

#!/usr/bin/env ruby
# frozen_string_literal: true

LOW_ALPHA = ('a'..'z')to_a.join.freeze
HIGH_ALPHA = ('A'..'Z')to_a.join.freeze

def caesar_cipher(str, shift)
low_encrypted = LOW_ALPHA.chars.rotate(shift).join.freeze
high_encrypted = HIGH_ALPHA.chars.rotate(shift).join.freeze

str.tr(LOW_ALPHA + HIGH_ALPHA, low_encrypted + high_encrypted)
end


# Final thoughts

Lastly, I just want to give you something to think about, without any comments from me:

# frozen_string_literal: true

class CaesarCipher
LOWER = ('a'..'z').to_a.join.freeze
UPPER = ('A'..'Z').to_a.join.freeze

def initialize(key)
self.encrypted = (LOWER.chars.rotate(key) + UPPER.chars.rotate(key)).join.freeze
end

def encrypt(str)
str.tr(LOWER + UPPER, encrypted)
end

alias_method :call, :encrypt

def to_proc
->str { encrypt(str) }
end

private

attr_accessor :encrypted

freeze
end

caesar5 = CaesarCipher.new(5)

puts caesar5.encrypt('What a string!') # Bmfy f xywnsl!
puts caesar5.('Abc') # Fgh
puts CaesarCipher.new(3).('Xyz') # Abc
puts CaesarCipher.new(1).('Test') # Uftu
puts CaesarCipher.new(10).('Zoo') # Jyy

puts ['What a string!', 'Abc'].map(&caesar5)