Mrthfx

What I'm trying to do: implement the Vigenère cipher in Ruby. I already have a working version, but I want to make sure it is efficient and well-designed.

module Crypto

  # Vigenère cipher encryption and decryption abstraction

  module Vigenere

    LETTERS = ('a'..'z').to_a.freeze

    private_constant :LETTERS



    module_function



    # Encrypts a string

    #

    # @param string [String] the string that will be encrypted

    # @param key [String] the key that will be used to encrypt the string

    #

    # @return [String] the encrypted string

    def encrypt(string:, key:)

      key = make_key(length: string.length, key: key)



      string.length.times.map { |i|

        p = LETTERS.find_index(string[i])

        k = LETTERS.find_index(key[i])



        LETTERS[(p + k) % 26]

      }.join

    end



    # Decrypts an encrypted string

    #

    # @param string [String] the encrypted string that will be decrypted

    # @param key [String] the key that will be used to decrypt the string

    #

    # @return [String] the decrypted string

    def decrypt(string:, key:)

      key = make_key(length: string.length, key: key)



      string.length.times.map { |i|

        c = LETTERS.find_index(string[i])

        k = LETTERS.find_index(key[i])



        LETTERS[(c - k + 26) % 26]

      }.join

    end



    # Repeats a word until it matches a certain length

    #

    # @param length [Integer] the length of the word being encrypted/decrypted

    # @param key [String] the word that will be repeated

    #

    # @return [String] the word in its new form

    def make_key(length:, key:)

      i = 0

      length.times do

        i = 0 if i == key.length

        break if key.length == length



        key << key[i]

        i += 1

      end



      key

    end



    private_class_method :make_key

  end

end

I do have some specific questions:

1. private_class_method

This is the best way I found to define a private method in a module, but it feels weird to me. Isn't there a better way to do that? My first implementation was this:

module Crypto

  class Vigenere

    class << self

      def encrypt # ...

      def decrypt # ...



      private



      def make_key # ...

    end

  end

end

which was fine for me. But then I read this rule on the Ruby Style Guide repository. So I switched to using module, but it doesn't feel right to use private methods in this structure. Am I wrong?

2. reseting a counter (index)

Take a look at this snippet of code:

def make_key(length:, key:)

  i = 0

  length.times do

    i = 0 if i == key.length

    break if key.length == length



    key << key[i]

    i += 1

  end



  key

end

Defining a counter (i) and manually incrementing it... looks awkward, doesn't it (at least in the Ruby world)? Is there a better way to do this?

3. valid multi-line block with curly braces?

Now take a look at this snippet:

string.length.times.map { |i|

  p = LETTERS.find_index(string[i])

  k = LETTERS.find_index(key[i])



  LETTERS[(p + k) % 26]

}.join

I know most Ruby developers tend to use curly braces only for one-line blocks and do-end for multi-line blocks, but this time it seems okay using curly braces with a multi-line block, since I'm chaining #join right after. What would you do:

1. use do-end, store it in a variable and invoke #join after that

new_letters = string.length.times.map do |i|

  p = LETTERS.find_index(string[i])

  k = LETTERS.find_index(key[i])



  LETTERS[(p + k) % 26]

end



new_letters.join

2. what I did (use curly braces even with multi-line block and chain #join)

And of course, if you have any other observations, please share.

The code is rather clear, so consider the following to be nitpicks.

I'd say that you are generating a new key stream from the key. I'd certainly not reuse the key variable.

The first i = 0 before the loop seems spurious.

Using i as a counter is well understood, and I'd not worry overly much on the style of it. You are probably the only one who cares if it is really Ruby-esk; developers down the line will understand it.

What I wonder though is that you run your loop length times, but there is a break that seems to trigger before that. That's not all too clear to me.

I wonder what happens if you supply it an "empty" key string. Some guard statements may be in order.

Same for the curly braces. It's clear as it is, choose whatever you want. Personally I slightly favor the braces.

You could consider creating a mod function, however since % is already the modulus, which will never return a negative value if the right value is positive, it seems to me that removing the + 26 is probably the only thing you need to change (during decryption).

Instead of using 26 as unexplained magic value, you should get the size of the LETTERS range instead. That way you can also expand your ciphertext later.

I've got no opinion on the private_class_method as I'm not a Ruby developer (I'm specialized in knowing many languages / constructs and of course applied crypto).

Looping using string.length.times.map { |i| … } and length.times do … is OK, but slightly on the awkward side.

To extend the key, you can use the string multiplication operator. (Note that extending the key longer than necessary doesn't do much harm.)

You can also factor out more of the commonality between the encrypt and decrypt functions.

Instead of searching the LETTERS array, I recommend performing arithmetic on ASCII codes.

module Crypto

  module Vigenere

    module_function

    def encrypt(plaintext, key) 

      vigenere(plaintext, key) { |p, k| (p + k) % 26 }

    end



    def decrypt(ciphertext, key) 

      vigenere(ciphertext, key) { |c, k| (c - k + 26) % 26 }

    end



    def vigenere(text, key, &combiner)

      a = 'a'.ord

      ext_key = key * (text.length / key.length + 1)

      text.chars.zip(ext_key.chars).collect do |t, k|

        (a + combiner.call(t.ord - a, k.ord - a)).chr

      end.join

    end

    private_class_method :vigenere

  end

end