Caesar Cipher → Bzdrzq Bhogdq

Question

I wrote this Caesar cipher and decipher just for the sake of practising OOP (previous answers to my java questions all seem to point to OOPness).

What it is:

The Caesar cipher is one of the earliest known and simplest ciphers. It is a type of substitution cipher in which each letter in the plaintext is 'shifted' a certain number of places down the alphabet.

Currently I have a Cipher interface and a CaesarCipher class: I am planning to add a Vigenère Cipher soon.

Code:

Cipher.java

public interface Cipher {

    public String cipher(String text);

    public String decipher(String cipherText);

    public char getCipheredChar(char textChar);

    public char getDecipheredChar(char cipherChar);

}

CaesarCipher.java

public class CaesarCipher implements Cipher {

    private static final int LETTERS = 'Z' - 'A' + 1;

    private int shift;

    public CaesarCipher(int shift) {
        if (shift < -LETTERS || shift > LETTERS) {
            throw new IllegalArgumentException("The value of shift: " + shift
                    + " is invalid.");
        }
        this.shift = shift;
    }

    public int getShift() {
        return shift;
    }

    @Override
    public String cipher(String text) {
        char[] result = text.toCharArray();
        for (int i = 0; i < result.length; i++) {
            result[i] = getCipheredChar(result[i]);
        }
        return new String(result);
    }

    @Override
    public String decipher(String cipherText) {
        this.shift = -shift;
        String result = cipher(cipherText);
        this.shift = -shift;
        return result;
    }

    @Override
    public char getCipheredChar(char textChar) {
        if (textChar > 'A' && textChar < 'z') {
            char result = (char) (textChar + shift);
            char compare = Character.isUpperCase(textChar) ? 'Z' : 'z';
            char lowCompare = Character.isUpperCase(textChar) ? 'A' : 'a';
            return (char) (result > compare ? result - 26
                    : (result < lowCompare ? result + 26 : result));
        }
        return textChar;
    }

    @Override
    public char getDecipheredChar(char cipherChar) {
        this.shift = -shift;
        char result = getCipheredChar(cipherChar);
        this.shift = -shift;
        return result;
    }

}

And tests!

import static org.junit.Assert.assertEquals;

import org.junit.Test;

public class TestCipher {

    @Test
    public void testShift1() {
        CaesarCipher cipher = new CaesarCipher(1);
        assertEquals("AaBbCc", cipher.decipher("BbCcDd"));
        assertEquals("Bzdrzq", cipher.decipher("Caesar"));
        assertEquals("Bzdrzq Bhogdq...", cipher.decipher("Caesar Cipher..."));
    }

    @Test
    public void testShift5() {
        CaesarCipher cipher = new CaesarCipher(5);
        assertEquals("AaBbCc", cipher.decipher("FfGgHh"));
        assertEquals("Xvznvm", cipher.decipher("Caesar"));
        assertEquals("Xvznvm Xdkczm...", cipher.decipher("Caesar Cipher..."));
    }

}

Concerns:

1. Are my tests fine?
2. Is my OOP structure good?
3. And as usual, anything else?

Answer 1

A few comments about your Cipher interface:

• A cipher is an algorithm to encrypt something, so your cipher method should be renamed to encrypt. To match encrypt, you should change decipher to decrypt.

• getCipherChar and getDecipheredChar is only really applicable to a few ciphers and not to a general cipher. For example, in the Viginere cipher, the encrypted character depends on the relative position compared to the repeated key. Therefore you can't tell what the result will be without additional information regarding where it is in the string. Other algorithms are more complex. These methods can still be used internally in CeasarCipher, but should be removed from the Cipher interface.

Regarding CeasarCipher:

• Chars aren't just A-Z and a-z, and the Cipher interface is a guarantee that you will encrypt/decrypt an arbitrary string. Currently any other character stays the same, which is not what the Ceasar cipher is. Therefore you should change your code to just increment a char's value by the shift, modulo Character.MAX_VALUE, with some care as a negative shift could lead to a negative modulo which can be fixed by calculating the final char as described in the following code:

---

char shiftedChar(char c, int shift):
    int ROLLOVER = Character.MAX_VALUE;
    return (((c + shift) % ROLLOVER) + ROLLOVER) % ROLLOVER;

Alternatively you could specify some sort of Alphabet type (which should be something like a List<Character>) which restricts the subset of Character's that you are interested in.

Answer 2

I don't recommend temporarily overwriting an instance variable like this:

@Override
public String decipher(String cipherText) {
    this.shift = -shift;
    String result = cipher(cipherText);
    this.shift = -shift;
    return result;
}

If an exception occurs in cipher(), then not only would you have that problem to deal with, but also another mysterious bug as a side-effect. Furthermore, this temporary change makes code that would otherwise be threadsafe become un-threadsafe.

Suggested alternative:

public String decipher(String cipherText) {
    return new CaesarCipher(-this.shift).encipher(cipherText);
}

Answer 3

Regarding tests, always try to come up with crazy input. What happens if we try cipher(null), cipher("") or cipher("#%(/¤=")?

Some of these inputs do not make sense in your cipher, but are not handled either.