Vigenere Cipher

This code asks the user to enter a message containing the lower case characters a-z, it then encrypts it into a Vigenere Cipher and also decrypts the cipher to prove the reverse lookup works.

```
package com.testing;

import java.util.Scanner;

/**
* A Vigenere Square or Vigenere table consists of the alphabet written out 26
* times in different rows, each alphabet shifted cyclically to the left
* compared to the previous alphabet, corresponding to the 26 possible Caesar
* ciphers, At different points in the encryption process, the cipher uses a
* different alphabet from one of the rows. The alphabet used at each point
* depends on a repeating keyword.
*/
final class VigenereSquare {

/**
* A 2D char array representing the shifted alphabets.
*/
public static final char[][] SQUARE = fillSquare();

private static final int LETTERS_IN_ALPHABET = 26;
private static final int ASCII_RANGE = 256;

private VigenereSquare() {}

/**
* Fill square with shifted alphabets in ASCII positions:
* 'a' = 97 .. 'z' = 122
* @return initialised char[][]
*/
private static char[][] fillSquare() {
char[][] square = new char[ASCII_RANGE][ASCII_RANGE];
int start = 'a';
int end = start + (LETTERS_IN_ALPHABET - 1);
int index = start;
for (int i = start; i end) {
index = start;
}
square[i][j] = (char) index;
index++;
}
index = i + 1;
}
return square;
}
}

/**
* The person sending the message to be encrypted (eg. attackatdawn) chooses a
* keyword and repeats it until it matches the length of the plaintext, for
* example, the keyword lemon, the cipher key will be lemonlemonle.
*/
class CipherKey {

/**
* CipherKey String value.
*/
public final String KEY;

public CipherKey(String text, String keyword) {
KEY = createKey(text, keyword);
}

/

Say what you mean

int start = 'a';
        int end = start + (LETTERS_IN_ALPHABET - 1);
        int index = start;
        for (int i = start; i  end) {
                    index = start;
                }
                square[i][j] = (char) index;
                index++;
            }
            index = i + 1;
        }

The purpose of this code seems to be to fill in the section of of the square corresponding to the letters in the alphabet. The start of the alphabet is always defined as 'a', but you calculate the end from the start and LETTERS_IN_ALPHABET. You then use both start and end as constants. Why not just make them constants and do away with LETTERS_IN_ALPHABET?

private static final char ALPHABET_START = 'a';
    private static final char ALPHABET_END = 'z';

Then we can just use those:

for (int i = ALPHABET_START; i  ALPHABET_END) {
                    c = ALPHABET_START;
                }

                square[i][j] = c;
                c++;
            }
        }

This is more flexible than the original, as we can alter both the start and end via the constants.

Also note that index is not actually an index. It's a letter, so either call it letter or something like c.

Since our new c variable is never used outside the i loop and is reset each iteration, simply define it inside the loop. Moving it from the end to the beginning means that we no longer need to set it to i + 1, as the beginning is after i is incremented. During the first iteration of the loop, c is set to ALPHABET_START, just as it was in the original code.

We also change c to be a char rather than an int, as that allows it to be directly used in the assignment to square[i][j] at the cost only of a cast outside the j loop.

The comment is now unnecessary, as the code reads like the comment did. If c is past the end of the alphabet, reset c to the start of the alphabet.

Consider giving an example, e.g.

* For an ALPHABET_START of 'a' and an ALPHABET_END of 'c', generate 
 * abc
 * bca
 * cab

Then it's easier to see that the progression is intentional and not accidental.

Say what you mean, again

StringBuilder key = new StringBuilder();
        for (int i = 0, keywordIndex = 0; i = keyword.length()) {
                keywordIndex = 0;
            }
            key.append(keyword.charAt(keywordIndex));
        }

The first thing to do here is to give the StringBuilder an initial capacity. We know the length, so tell the code.

StringBuilder key = new StringBuilder(text.length());

This allows the compiler to allocate the correct length of StringBuilder at the beginning rather than picking an arbitrary length and expanding it as necessary.

This code is written similarly to how the previous code was written, but it does something different. What it's doing is appending keyword to key until it's the same length as text. So just do that.

final int fullCount = text.length() / keyword.length();
        for (int i = 0; i < fullCount; i++) {
            key.append(keyword);
        }

        final int remainingLength = text.length() % keyword.length();
        key.append(keyword.substring(0, remainingLength));

Rather than appending character by character, we append whole copies of the string. This saves the problem of maintaining keywordIndex.

Multiple files

In Java, it's standard to put each class in its own file. This makes it easier to reuse classes, as you can copy just the files that you need.

Testing

I tested this code with your test case and a couple others:


abc

de


abcd

ef


abc

gfed

It returns the same results as your code for the first case and your test case. I didn't check the others against your code, as I thought of them after I made modifications. They all produce reasonable output and echo the original string.

This is an argument in favor of published unit tests. If you had already been testing a number of circumstances like this, I could have just used your tests. Then I'd be reasonably sure that both versions did the same thing. That makes it easier to make modifications with confidence that they won't cause regressions.

Note: I'm not commenting on this method of encryption. Good? Bad? I'm not the right person to say. My comments are mainly aimed at readability with a slight nod to performance.

StackExchange Code Review Q#88864, answer score: 4