11
\$\begingroup\$

I wrote this Caesar cipher and decipher just for the sake of practising OOP (previous answers to my 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?
\$\endgroup\$
18
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why use "encrypt" when it would make more sense to name it "encipher"? \$\endgroup\$ – ArtOfCode Nov 30 '15 at 14:23
  • 2
    \$\begingroup\$ @ArtOfCode I've rarely heard people say encipher while encrypt/decrypt is quite common. In any case, as long as its not just "cipher", which isn't even a verb it's fine. \$\endgroup\$ – mleyfman Nov 30 '15 at 14:42
  • \$\begingroup\$ perhaps it's just location differences, then. Where I am, encipher is quite common. \$\endgroup\$ – ArtOfCode Nov 30 '15 at 14:43
  • \$\begingroup\$ @ArtOfCode I have to agree with mleyfman on this one. This might be nit picking, but code should be as universal as much as possible, if we start localizing all the internals of a code base too much, it'll make it difficult for future consumers/users of the code who aren't local. My home language is Afrikaans, but if I start naming all my variables in Afrikaans it would make it impossible for more than 90% of the world to read (easily) \$\endgroup\$ – Rohan Büchner Dec 1 '15 at 5:57
15
\$\begingroup\$

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);
}
| improve this answer | |
\$\endgroup\$
  • 5
    \$\begingroup\$ It's safer if a private getCipheredChar function is created that takes the shift as a parameter and both public cipher and decipher forward to that. \$\endgroup\$ – ratchet freak Nov 30 '15 at 9:27
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.