8
\$\begingroup\$

I must implement a simple Caesar cipher class. Initializer takes two parameters - number of positions to shift the alphabet (integer) and the alphabet used (as string, not necessarily the standard English alphabet). The class must have two instance methods encrypt and decrypt, which take a string and do the respective action based on the parameters of the initializer. These are my rspec tests:

describe "Caesar" do
 latin_encrypter = Caesar.new 4
 dna_encrypter = Caesar.new 3, 'ACTG'
 it 'encrypts correctly' do
 expect(latin_encrypter.encrypt 'hello').to eq "lipps"
 end
 it 'encrypts correctly using DNA alphabet' do
 expect(dna_encrypter.encrypt 'ACCTGA').to eq "GAACTG"
 end
 it 'decrypts correctly' do
 expect(latin_encrypter.decrypt 'lipps').to eq 'hello'
 end
 it 'decrypts correctly using DNA alphabet' do
 expect(dna_encrypter.decrypt 'GAACTG').to eq 'ACCTGA'
 end
end

and this is my implementation:

class Caesar
 def initialize(shift, alphabet = 'abcdefghijklmnopqrstuvwxyz')
 @shift = shift % alphabet.size
 @alphabet = alphabet
 end
 def encrypt(string)
 string.chars.map { |char| @alphabet[@alphabet.index(char) + @shift - @alphabet.size] }.join
 end
 def decrypt(string)
 string.chars.map { |char| @alphabet[@alphabet.index(char) - @shift] }.join
 end
end

Is this an optimal algorithm for encryption/decryption or is there a better one (perhaps using gsub)?

200_success
146k22 gold badges190 silver badges479 bronze badges
asked Nov 13, 2013 at 22:34
\$\endgroup\$
1
  • \$\begingroup\$ Caesar.new(1).encrypt('1337')NoMethodError: undefined method `+' for nil:NilClass. The parameter may be invalid, but the error message should be more helpful. \$\endgroup\$ Commented Nov 13, 2013 at 23:18

3 Answers 3

4
\$\begingroup\$

Ruby has tr, a very efficient method for substituting one character to another. It does not error on "unknown" characters, like spaces etc. Using that, the Caesar class becomes:

class Caesar
 def initialize(shift, alphabet = ('a'..'z').to_a.join)
 i = shift % alphabet.size #I like this
 @decrypt = alphabet
 @encrypt = alphabet[i..-1] + alphabet[0...i]
 end
 def encrypt(string)
 string.tr(@decrypt, @encrypt)
 end
 def decrypt(string)
 string.tr(@encrypt, @decrypt)
 end
end
answered Nov 16, 2013 at 23:19
\$\endgroup\$
4
\$\begingroup\$

The only (conceptual) problem I see in your code is that both encrypt/decrypt perform a \$O(n)\$ operation (detect) for every character (so at then end it's a \$O(n*m)\$ algorithm), that's unnecessarily inefficient. Build a hash object to use as an indexer:

class Caesar
 def initialize(shift, alphabet = ('a'..'z').to_a.join)
 @shift = shift
 @alphabet = alphabet
 @indexes = alphabet.chars.map.with_index.to_h
 end
 def encrypt(string)
 string.chars.map { |c| @alphabet[(@indexes[c] + @shift) % @alphabet.size] }.join
 end
 def decrypt(string)
 string.chars.map { |c| @alphabet[(@indexes[c] - @shift) % @alphabet.size] }.join
 end
end

You can also build encrypter/decrypter hash tables at initialization:

class Caesar
 def initialize(shift, alphabet = ('a'..'z').to_a.join)
 chars = alphabet.chars.to_a
 @encrypter = chars.zip(chars.rotate(shift)).to_h
 @decrypter = @encrypter.invert
 end
 def encrypt(string)
 @encrypter.values_at(*string.chars).join
 end
 def decrypt(string)
 @decrypter.values_at(*string.chars).join
 end
end
Sᴀᴍ Onᴇᴌᴀ
29.6k16 gold badges45 silver badges203 bronze badges
answered Nov 13, 2013 at 22:47
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Yes, exactly, Array#rotate was the thing I was looking for. Thank you very much! The second version is really elegant! \$\endgroup\$ Commented Nov 13, 2013 at 23:01
-2
\$\begingroup\$

Your code does not work with letters like "A..Z". The following does:

puts "Text for encrypt please: "
text_for_encrypt = gets.chomp
puts "Input encrypt key: "
key = gets.chomp.to_i
text_arrow = text_for_encrypt.split('').to_a
alphabet_az = ("a".."z").to_a.join
alphabet_AZ = ("A".."Z").to_a.join
i = key % alphabet_az.size
encrypt_az = alphabet_az.chars.rotate(i).join
encrypt_AZ = alphabet_AZ.chars.rotate(i).join
res = []
text_arrow.each do |letter|
 if ("a".."z") === letter
 letter = letter.tr( alphabet_az, encrypt_az )
 res << letter
 else
 letter = letter.tr( alphabet_AZ, encrypt_AZ )
 res << letter
 end
end
puts "Your code: #{res.join}"
Peilonrayz
44.4k7 gold badges80 silver badges157 bronze badges
answered Mar 2, 2017 at 14:45
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to StackExchange Code Review! Please see: How do I write a good answer?, where you will find: "Every answer must make at least one insightful observation about the code in the question. Answers that merely provide an alternate solution with no explanation or justification do not constitute valid Code Review answers and may be deleted". \$\endgroup\$ Commented Mar 2, 2017 at 15:10

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.