1
\$\begingroup\$

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?

\$\endgroup\$
  • \$\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\$ – Miguel Avila Jun 21 at 17:48
3
\$\begingroup\$

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)
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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