2
\$\begingroup\$

I am going through the Odin Project Ruby section, and just finished implementing a Caesar cipher interpretation. Should I use classes, or more than one method to distribute the flow? Is there really a need for initialization in this?

def caesar_cipher(string, shifter)
    ciphered = []
    string.split("").each do |char|
        if char == " " then ciphered << char 
            next 
        end

        char = char.to_s.ord

        if ("a".ord.."z".ord).include? char
            if char + shifter < "z".ord
                ciphered.push((char + shifter).chr)
            else
                ciphered.push((shifter - ("z".ord - char) + "a".ord).chr)
            end         
        elsif ("A".ord.."Z".ord).include? char
            if char + shifter < "Z".ord
                ciphered.push((char + shifter).chr)
            else
                ciphered.push((shifter - ("Z".ord - char) + "A".ord).chr)
            end 
        else    
            next
        end

    end
    print ciphered.join + "\n"
end

#test: caesar_cipher("Hello friend my Name is Sam",10)
\$\endgroup\$
1

1 Answer 1

3
\$\begingroup\$

The code can be simplified, but you're on the right track.

A function is sufficient for a simple string filter like this; a class would be overkill. I would rename the first parameter to plaintext and the result to ciphertext because that is the standard terminology.

The function should return the ciphertext rather than printing it. The caller should have the flexibility to print it, show it in a GUI, store it in a database, or whatever.

Instead of split("").each, you can iterate over the plaintext using plaintext#each_codepoint. You can also build the result by appending to a string rather than joining a list.

The if…elseif would be slightly better as a case expression. You don't need the special case for " "

def caesar_cipher(plaintext, shift)
  ciphertext = ''
  plaintext.each_codepoint do |asc|
    ciphertext << case asc
      when 'a'.ord..'z'.ord
        'a'.ord + (asc - 'a'.ord + shift) % 26
      when 'A'.ord..'Z'.ord
        'A'.ord + (asc - 'A'.ord + shift) % 26
      else
        asc
    end
  end
  ciphertext
end
\$\endgroup\$
4
  • \$\begingroup\$ This is great, thank you! What does the each_codepoint method do? \$\endgroup\$
    – ppadru1
    Feb 20, 2015 at 1:40
  • 1
    \$\begingroup\$ String#each_codepoint is like each_char but yields the ord of each character. \$\endgroup\$ Feb 20, 2015 at 2:42
  • \$\begingroup\$ That is awesome, thanks! Also, I forgot the syntax of the case when statement that in each when you just specify the condition and don't need to repeat "asc in..". Very useful! \$\endgroup\$
    – ppadru1
    Feb 20, 2015 at 16:42
  • \$\begingroup\$ This is a good answer. It might be possible to simplify further by using map to obviate the need for the intermediary ciphertext variable. \$\endgroup\$ Feb 22, 2015 at 1:01

Your Answer

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

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