Substitution Cipher Machine

Question

Here is an attempt I made at a simple Substitution based cipher machine, it shifts input characters by an amount then returns an unreadable string which can then be decrypted back to its original self:

package Pack;

import java.io.UnsupportedEncodingException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;

/**
 * Start of CiphermachineMK3.
 **/
public class CipherMachineMK3 {

    /**
     * Encryption character map before Encryption.
     */
    private List<String> encryptMappingfrom = Arrays.asList(":", "/", "?", "#", ".", " ");

    /**
     * Encryption character map after Encryption.
     */
    private List<String> encryptMappingto = Arrays.asList("!", "-", "+", ",", "]", " ");

    /**
     * Decryption character map before Decryption.
     */
    private List<String> decryptMappingfrom = Arrays.asList("!", "-", "+", ",", "]", " ");

    /**
     * Decryption character map after Decryption.
     */
    private List<String> decryptMappingto = Arrays.asList(":", "/", "?", "#", ".", " ");

    /**
     * HashMap to switch inputted character with encryption value.
     */
    private static HashMap<String, String> encryptMapping = new HashMap<>();

    /**
     * HashMap to switch inputted character with decryption value.
     */
    private static HashMap<String, String> decryptMapping = new HashMap<>();

    /**
     * Amount of shift required to loop the characters of the alphabet from A-Z and vice
     * versa.
     */
    private static final int SHIFT_ROTATE_VALUE = 26;

    /**
     * Encoding shift amount.
     */
    private static final int NUMBER_TO_SHIFT_DIGITS = 3;

    /**
     * Shift amount required for decoding challenge.
     */
    private static final int SECRET_DECODE_VALUE = 18;

    /**
     * Storage for lower case alphabet variables.
     */

    private static final String ALPHABET_LOWERCASE = "abcdefghijklmnopqrstuvwxyz";

    /**
     * Storage for upper case alphabet variables.
     */
    private static final String ALPHABET_UPPERCASE = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

    /**
     * Mapping for the encoding of specific characters.
     * 
     * @param encryptMappingfrom
     *            Characters to the be encrypted.
     * @param encryptMappingto
     *            Characters encrypted to.
     * @return Encryption map
     */
    static HashMap<String, String> encryptFixedmappings(final List<String> encryptMappingfrom,
                    final List<String> encryptMappingto) {

        for (int i = 0; i < encryptMappingfrom.size(); i++) {

            encryptMapping.put(encryptMappingfrom.get(i), encryptMappingto.get(i));
        }
        return encryptMapping;

    }

    /**
     * Mapping for the decoding of specific characters.
     * 
     * @param decryptMappingfrom
     *            Characters to be decrypted.
     * @param decryptMappingto
     *            Characters decrypted to.
     * @return Decryption map
     */
    static HashMap<String, String> decryptFixedmappings(final List<String> decryptMappingfrom,
                    final List<String> decryptMappingto) {

        for (int i = 0; i < decryptMappingfrom.size(); i++) {
            decryptMapping.put(decryptMappingfrom.get(i), decryptMappingto.get(i));
        }
        return decryptMapping;

    }

    /**
     * Start of Encrypt method.
     * 
     * @param plainText
     *            Input of clear readable text.
     * @param shiftKey
     *            Amount to shift text by when encoding.
     * @return cipherText The final encoded result.
     */
    public String encrypt(final String plainText, final int shiftKey) {
        String cipherText = "";
        String charasString = "";
        encryptMapping = CipherMachineMK3.encryptFixedmappings(encryptMappingfrom, encryptMappingto);
        if (plainText == null) {
            return null;
        }
        // Loop begins
        for (int i = 0; i < plainText.length(); i++) {
            // Checks if character is alphabetic
            if (Character.isAlphabetic(plainText.charAt(i))) {

                    // Performs tasks if character is Lower-case
                    if (Character.isLowerCase(plainText.charAt(i))) {
                        int lowerCharPosition = ALPHABET_LOWERCASE.indexOf(plainText.charAt(i));

                        int keyVal = (shiftKey + lowerCharPosition) % SHIFT_ROTATE_VALUE;
                        if (keyVal < 0) {
                            keyVal = ALPHABET_LOWERCASE.length() + keyVal;
                        }
                        char sortEncryptLowerCaseASCII = (char) keyVal;
                        char replaceVal = ALPHABET_LOWERCASE.charAt(sortEncryptLowerCaseASCII);

                        cipherText += replaceVal;
                    }
                    // Performs tasks if character is Upper-case
                    if (Character.isUpperCase(plainText.charAt(i))) {

                        int upperCharPosition = ALPHABET_UPPERCASE.indexOf(plainText.charAt(i));

                        int keyVal = (shiftKey + upperCharPosition) % SHIFT_ROTATE_VALUE;
                        if (keyVal < 0) {
                            keyVal = ALPHABET_UPPERCASE.length() + keyVal;
                        }
                        char sortEncryptUpperCaseASCII = (char) keyVal;
                        char replaceVal = ALPHABET_UPPERCASE.charAt(sortEncryptUpperCaseASCII);

                        cipherText += replaceVal;

                    }

                } else {

                // Begins symbol checker loop
                if (Character.isWhitespace(plainText.charAt(i))) {
                    cipherText += plainText.charAt(i);
                    continue;
                } else {
                    charasString = String.valueOf(plainText.charAt(i));
                    if (!encryptMapping.containsKey(charasString)) {
                        // Default value for unrecognised characters
                        cipherText += "_";
                    } else {
                        cipherText += encryptMapping.get(charasString);
                    }
                }
                // Checks if character is alphabetic
            }

        }
        return cipherText;
    }

    /**
     * Start of Decrypt method.
     * 
     * @param cipherText
     *            Input of encoded text.
     * @param shiftKey
     *            Amount to shift text by when decoding.
     * @return plainText The final decoded result.
     */
    public String decrypt(final String cipherText, final int shiftKey) {
        String plainText = "";
        String charasString = "";
        decryptMapping = CipherMachineMK3.decryptFixedmappings(decryptMappingfrom, decryptMappingto);
        if (cipherText == null) {
            return null;
        }
        // Loop begins
        for (int i = 0; i < cipherText.length(); i++) {

            // Checks if character is alphabetic
            if (Character.isAlphabetic(cipherText.charAt(i))) {
                // Performs tasks if character is Lower-case
                if (Character.isLowerCase(cipherText.charAt(i))) {
                    int charPosition = ALPHABET_LOWERCASE.indexOf(cipherText.charAt(i));

                    int keyVal = (charPosition - shiftKey) % SHIFT_ROTATE_VALUE;
                    if (keyVal < 0) {
                        keyVal = ALPHABET_LOWERCASE.length() + keyVal;
                    }
                    char sortDecryptLowerCaseASCII = (char) keyVal;
                    char replaceVal = ALPHABET_LOWERCASE.charAt(sortDecryptLowerCaseASCII);
                    plainText += replaceVal;
                }
                // Performs tasks if character is Upper-case
                if (Character.isUpperCase(cipherText.charAt(i))) {
                    int charPosition = ALPHABET_UPPERCASE.indexOf(cipherText.charAt(i));
                    int keyVal = (charPosition - shiftKey) % SHIFT_ROTATE_VALUE;
                    if (keyVal < 0) {
                        keyVal = ALPHABET_UPPERCASE.length() + keyVal;
                    }
                    char sortDecryptUpperCaseASCII = (char) keyVal;
                    char replaceVal = ALPHABET_UPPERCASE.charAt(sortDecryptUpperCaseASCII);
                    plainText += replaceVal;
                }
            }
            // Checks if character is non-alphabetic
                        if (!Character.isAlphabetic(cipherText.charAt(i))) {
                            // Begins symbol checker loop
                            if (Character.isWhitespace(cipherText.charAt(i))) {
                                plainText += cipherText.charAt(i);
                                continue;
                            } else {
                                charasString = String.valueOf(cipherText.charAt(i));
                                if (!decryptMapping.containsKey(charasString)) {
                                    // Default value for unrecognised characters
                                    plainText += "*";
                                }

                                plainText += decryptMapping.get(charasString);
                            }

                        }
        }
        return plainText;
    }

    /**
     * Start of main method.
     * 
     * @param args
     *            args
     * @throws UnsupportedEncodingException Throws exception in case string to byte conversion fails.
     */
    public static void main(final String[] args) throws UnsupportedEncodingException {

            CipherMachineMK3 cipherMachine = new CipherMachineMK3();
            System.out.println(cipherMachine.encrypt("Hello World", NUMBER_TO_SHIFT_DIGITS));
            System.out.println(cipherMachine.encrypt("Well done: #Decoded am i right?", NUMBER_TO_SHIFT_DIGITS));
            System.out.println(cipherMachine.decrypt("Khoor Zruog", NUMBER_TO_SHIFT_DIGITS));
            System.out.println(cipherMachine.decrypt("Ugfyjslmdslagfk ]]] Lwdd fgtgvq lzwhskkogjv ak! ,Uzwvvsj",
                            SECRET_DECODE_VALUE));

    }
}

Written in Eclipse. Any feedback regarding potential improvements or even just "Get rid of X and do X instead" would be greatly appreciated.

Answer 1

• First, about your two strings ALPHABET_LOWERCASE and ALPHABET_UPPERCASE. You are re-inventing the wheel, because the alphabetical ordering of the 26 letters is already reflected by the values of the code points these letters have been assigned to in the Unicode standard: The letters "A" to "Z" correspond to the code points U+0041 to U+005A, and the letters "a" to "z" correspond to the code points U+0061 to U+007A (the code points are typically referred to by the hexadecimal representation of their values). And a char in Java is nothing but a 16-bit value that contains the value of this Unicode code point. For example, the assignments char a = 'a' and char a = 0x61 (or char a = 97, which would be the decimal representation) are equivalent, and you can perform mathematical operations on chars just like you can with other primitive values that represent integers.

  This means that, in order to represent the alphabet, you don't need to store every single character. In fact, for the purposes of your code, I don't think it is necessary to store anything, since you only use the two strings for shifting a character by a certain number of positions in the alphabet, and you can do that using simple addition. For example, to shift an uppercase character by 3 positions, you can just do this:

  char encryptedChar = (char) ((originalChar - 'A' + 3) % 26 + 'A')
  

  The possibility of the shift key being negative makes this a bit more complicated, because chars can only have positive values. On the other hand, in the expression originalChar - 'A' + 3 above, originalChar and 'A' would first be converted to ints before the expression originalChar - 'A' is evaluated. Here is a link to Chapter 4.2.2. from the Java Language Specification where this is explained. The following two paragraphs are relevant:

  If an integer operator other than a shift operator has at least one operand of type long, then […].

  Otherwise, the operation is carried out using 32-bit precision, and the result of the numerical operator is of type int. If either operand is not an int, it is first widened to type int by numeric promotion.

  To illustrate this, here is a code sample:

  char zeroChar = 0;
  int testInt = zeroChar - 1;     // -1
  char testChar = (char) testInt; // 65535
  

  In the second line, zeroChar is converted to an int before 1 is subtracted from it, so no overflow occurs. But since -1 cannot be represented by a char, information is lost in the narrowing conversion from int to char in the third line, and the result is \$2^{16}-1\$, which is the maximum value of a char.

• You seem to be under the impression that Character.isAlphabetic(int) checks whether a character is one of the 52 characters from "A" to "Z" and from "a" to "z". This is not true, as the method's documentation reveals. There are many more characters that are considered alphabetic by this method which your code does not deal with. Likewise, the methods Character.isLowerCase(char) and Character.isUpperCase(char) return true for many more upper- and lowercase characters than your code deals with. So if you want to check whether a character is one of the 52 characters mentioned above, this can easily be done with relational operators like > and <, taking into account what I have explained in the previous point.

• Why is the variable sortEncryptLowerCaseASCII a char and not an int? It doesn't make a difference for the result of the program, but it is confusing, because sortEncryptLowerCaseASCII represents the index of a character in a string, not a character itself, and when you pass it to String.charAt(int), it is implicitly cast to an int anyway, so there is absolutely no point in making this variable a char.

• Instead of constructs like these:

  if (Character.isAlphabetic(plainText.charAt(i))) {
      // ...
  } else {
      if (Character.isWhitespace(plainText.charAt(i))) {
          // ...
      } else {
          // ...
      }
  }
  

  You can write this:

  if (Character.isAlphabetic(plainText.charAt(i))) {
      // ...
  } else if (Character.isWhitespace(plainText.charAt(i))) {
      // ...
  } else {
      // ...
  }
  

  This reduces the level of nesting, which makes the flow of the program a bit easier to follow.

• Your code contains a lot of duplicate code. For example, the decryption and encryption methods perform essentially the same operations. From what I can tell, the only differences are the character mappings for non-alphabetic characters, the replacement character for unrecognized characters, and whether you add or subtract the shift key when converting alphabetic characters. So instead of duplicating the conversion algorithm in two methods, you could just make one method that accepts the character mapping and the replacement character as a parameter and adds the shift key, and then create two wrapper methods for decryption and encryption that merely call this method with the appropriate parameters (which would include the negated shift key when calling the method for a decryption).

  Likewise, the two blocks for converting upper- and lowercase characters are almost identical. You should try to extract the common functionality to one place and isolate the differences in order to eliminate as much duplicate code as possible. The only difference here is whether you use the String ALPHABET_LOWERCASE or ALPHABET_UPPERCASE. So you could declare a local variable String referenceString, which gets assigned ALPHABET_LOWERCASE if the character is lowercase, and ALPHABET_UPPERCASE if the character is uppercase, and then, you just replace every occurrence of ALPHABET_LOWERCASE/ALPHABET_UPPERCASE with referenceString and you can annihilate the duplicate code.

• A more appropriate type for your List<String> fields would be List<Character>. This should be self-explanatory.

• Finally, your usage of static and non-static fields and methods is very confusing, and I don't know if there is a purpose behind this or if you just didn't know what you were doing. I don't really know how to review this aspect of your code, because I cannot discern a meaning behind your choices of what to make static and what to make non-static. For instance, the four lists containing the non-alphabetic characters are instance variables, but they only ever carry the completely instance-independent value to which they were initialized, rendering the fact that they are non-static variables absurd. The HashMaps on the other hand, which are needed for encryption and decryption, are static, yet the encrypt and decrypt methods are non-static.

  You might not have understood the purpose of static and non-static fields and methods, and I don't think this example with the cipher machine lends itself to explaining the difference very well. You might take a look at this. I'm sure there are also plenty of other resources available on the internet.