7
\$\begingroup\$

I have built a Caesar Cipher in Ruby that utilizes ASCII for the encryption. I am not sure whether or not this was the best way to go about the task but this way made the most sense to me. Please give me any feedback that comes to mind.

puts 'Please enter your string to be coded'
    string = gets.chomp.split("")

puts 'Please enter your number key'
    key = gets.chomp.to_i

    if (key > 26) 
        key = key % 26
    end

    string.map! do |i|
        i.ord
    end

    string.map! {|i| i = i + key}.map! {|i| 
        if (i > 122)
            then i = i - 26
        elsif (90 < i && i < 97)
            then i = i - 26
        elsif (i > 96 && (i - key) < 91 && (i - key) > 64)
            then i = i - 26
        elsif (i < 65 )
            then i = 32
        else
            i
        end
    }

    string.map! do |i|
        i.chr 
    end

    puts "Your coded word is #{string.join("")}"

puts 'Return back to original string? (Yes or No)'
    response = gets.chomp.downcase

    if response == 'yes'
        string.to_s.split("")
        string.map! do |i|
            i.ord
        end

        string.map! {|i| i - key}.map! {|i| 
            if (90 < i && i < 97)
                then i = i + 26
            elsif ((i+key) > 96 && i < 91 && i > 64)
                then i = i + 26
            elsif (i < 65 && (i+key) < 91 && (i + key) > 64)
                then i = i + 26
            elsif (i < 65)
                then i = 32
            else
                i
            end
        }

        string.map! do |i|
            i.chr
        end

        puts string.join("")

    end
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to Codereview, Good first question! :) \$\endgroup\$ – Caridorc Aug 19 '15 at 22:29
  • \$\begingroup\$ Hops, this is the second \$\endgroup\$ – Caridorc Aug 20 '15 at 10:19
4
\$\begingroup\$

Your code is one block, one piece of code that goes from start to finish, mixing logic and user-interface. While it works (Great!) this is sub-optimal for readibility and maintanability.

I will now show you what I think is a more readable approach. (The concept is the same, but this code will behave a bit differently than yours. Adapting it to your needs is left as an exercise for the reader).

Without farther ado, let's start:

 require 'arrow_test'

This code imports an automated testing framework I wrote, inatalling it is as easy as gem install arrow_test in the command line. Automated tests run in milliseconds to verify the correctness of your code.

ALPHABET = ("a".."z").to_a

The alphabet. Note that UPPERCASE means that it cannot be changed. It is a constant.

# Moves a letter forward in the alphabet by
# the given key wrapping around.
#

A short summary of what the function does, method level comments are encouraged over line level comments.

# <code> 
# shift('a', 2) #=> 'c' 
# </code>
#
# <code> 
# shift('z', 2) #=> 'b' 
# </code>

Example usage that doubles as tests, you may change them to see what happens.

 def shift(letter, key)
     ALPHABET[ (ALPHABET.index(letter) + key) % ALPHABET.length]
 end

This is a function, takes an input and has an output, It applies the Caesar cipher logic to a single character. The modulo operator % makes it behave like the alphabet is a circle. You may see the example with z.

 # Encrypts a text using by shifting all its letters by
 # the given key. All characters that are not lower and alphabetic
 # are deleted.
 # 
 # <code>
 # monoalphabetic_cipher('abcd', 2) #=> 'cdef'
 # </code>
 # 
 # <code>
 # monoalphabetic_cipher('cdef', 2, decode=true) #=> 'abcd'
 # </code>

A short phrase of description and a pair of tests like the other function.

 def monoalphabetic_cipher(text, key, decode=false)
   text
    .chars       
    .select { |char| ALPHABET.include?(char) }
    .map {|char| shift(char, (decode ? 26 - key : key) )}
    .join
 end

Here I discard the non-lowercase chars, (select line), and apply the shift to each char (map line). chars and join are just to go from string to list and viceversa, not important. Seeing a 'flowchart' of the code may help understanding.

A programatically generated flowchart, showing all the steps.

And this is all, try it with:

puts 'Enter shift and then text'
shift = gets.chomp.to_i
text = gets.chomp
puts monoalphabetic_cipher(text, shift)

Now user interface and logic are separated and the logic is tested. Feel free to ask any question

\$\endgroup\$
4
  • \$\begingroup\$ Very elegant and simple. Thank you @Caridorc. And if i wanted this to work for capitals in the string, would I just make a separate capitals array and method? \$\endgroup\$ – scobo Aug 20 '15 at 11:32
  • \$\begingroup\$ IIRC there are no actual constants in Ruby, right? The ALLCAPS is just a coding convention to let the maintainer know that they shouldn't change its value, correct? \$\endgroup\$ – RubberDuck Aug 20 '15 at 11:42
  • 1
    \$\begingroup\$ @RubberDuck Changing a constant issues a warning, so it is a convention acknowlegded by the interpreter. The code will run fine though. \$\endgroup\$ – Caridorc Aug 20 '15 at 11:46
  • \$\begingroup\$ @scobo You may use .downcase to convert uppercase to lowercase at the start, or you may ignore all but lower letters in the encoding. \$\endgroup\$ – Caridorc Aug 20 '15 at 11:48

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.