Caesar Cipher → Bzdrzq Bhogdq

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) {
            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  'A' && textChar  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("Bz

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) which restricts the subset of Character's that you are interested in.

StackExchange Code Review Q#112274, answer score: 19