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]
            ciphered << char


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?

  • \$\begingroup\$ 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. \$\endgroup\$ Commented Jun 21, 2020 at 17:48

1 Answer 1



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 = +''


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?.


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   


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

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.


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

chars = str.chars


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

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


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


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)

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)

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)

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)


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

# => ["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:

# => [["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)

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

  def encrypt(str) = str.tr(LOWER + UPPER, encrypted)
  alias_method :call, :encrypt

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


  attr_accessor :encrypted


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)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.