Caesar cipher - Encode text from external file using HashMap Java

Question

This is one of my school assignments and I just wanted to ask if there are some obvious mistakes or things I could maybe improve in this code. Thank you for any suggestions.

import java.io.File; import java.io.FileNotFoundException; import java.util.HashMap; import java.util.Map; import java.util.Scanner; public class App { public static void main(String[] args) { //Initialize and populate alphabet HashMap HashMap<Integer, Character> alphabet = new HashMap<Integer, Character>(); alphabet.put(1, 'A'); alphabet.put(2, 'B'); alphabet.put(3, 'C'); alphabet.put(4, 'D'); alphabet.put(5, 'E'); alphabet.put(6, 'F'); alphabet.put(7, 'G'); alphabet.put(8, 'H'); alphabet.put(9, 'I'); alphabet.put(10,'J'); alphabet.put(11, 'K'); alphabet.put(12, 'L'); alphabet.put(13, 'M'); alphabet.put(14, 'N'); alphabet.put(15, 'O'); alphabet.put(16, 'P'); alphabet.put(17, 'Q'); alphabet.put(18, 'R'); alphabet.put(19, 'S'); alphabet.put(20, 'T'); alphabet.put(21, 'U'); alphabet.put(22, 'V'); alphabet.put(23, 'W'); alphabet.put(24, 'X'); alphabet.put(25, 'Y'); alphabet.put(26, 'Z'); try { //Load file File ciphertext = new File("ciphertext.txt"); Scanner reader = new Scanner(ciphertext); //Input for key Scanner keyboard = new Scanner(System.in); System.out.println("Enter the key: "); int key = keyboard.nextInt(); //Main while loop while (reader.hasNext()) { String data = reader.nextLine(); String decrypted = ""; //Loop through ciphertext.txt for (int i = 0; i < data.length(); i++) { //If current character is a space, append space to decrypted if (data.charAt(i) == ' '){ decrypted += ' '; } //For each element in alphabet for (Map.Entry<Integer, Character> entry : alphabet.entrySet()) { //If entry == curr char in data (entry = one alphabet letter) if (entry.getValue() == data.charAt(i)) { //If entry + key is greater than 26 if (entry.getKey() + key > 26) { //Total key - 26 decrypted += alphabet.get(entry.getKey() + key - 26); } else { decrypted += alphabet.get(entry.getKey() + key); } } } } System.out.println(decrypted); } //Close reader reader.close(); //File not found exception } catch (FileNotFoundException e) { System.out.println("File not found"); e.printStackTrace(); } } 

}

Answer 1

1. Fix your indentation.
2. Think more about names. key doesn't tell us nearly as much as rotation would, for example.
3. A HashMap with sequential numeric keys is equivalent to an array but the array would be more efficient.
4. Start your indexes at 0, not 1.
5. Learn about the % operator.
6. Handle both upper and lower case.
7. If you've handled a space, you needn't do the lookup. But it probably makes more sense to lookup and copy across unmatched characters unchanged.
8. Don't build your result by concatenation. Use a StringBuilder.

But there is really little point in the lookup. Chars in Java are integer values, so this can all be done by simple arithmetic.

Answer 2

Apart from already mentioned:

• Split your code into more methods that handle simpler things. For example method, that just does the cipher for a string. A method, that will read all the input. I cannot use your code to cipher something just like that, it is too closely bound to file loading.
• I wouldn't try to catch FileNotFoundException, let it bubble up, it will give similar message to the user.
• Your error handling in regards to closing stream is bad. Your stream(reader) should close no matter what. Either use try with resources or always close in a finally block.
• Consider all the characters that probably don't need encoding apart from space (maybe comma, colon, semicolon, question mark, exclamation mark?). Adjust the logic accordingly - probably that if the character is not in your mapping data structure, do not encode it.
• I like the usage of HashMap. Array may be more efficient, but you are then limited by the fact, that it starts by 0 and you have to use all indexes, etc. In HashMap you can use only the keys you are interested in. Still, you can construct this data more efficiently rather than hard-coding all the values. You can also use ASCII table mapping if viable ('A' = 65, etc). I would consider using it for mapping from the original character to deciphered character. That makes your code easier and you can also easily decipher using reverse map :)

Answer 3

I want to concentrate on the use of primitive type hash map, because it is a common anti-pattern that makes code very hard to read.

//Initialize and populate alphabet HashMap HashMap<Integer, Character> alphabet = new HashMap<Integer, Character>(); alphabet.put(1, 'A'); alphabet.put(2, 'B'); 

Nothing in this code describes the purpose of the hash map or the relation between the keys and the values. Because the keys are primitive types, their purpose cannot be derived from the type either. It takes quite a lot of time to figure out that the keys are just used as makeshift array indices.

Often it is a better idea to wrap the hash map into a dedicated container that has domain specific functions for accessing the map. In this specific instance you would create a CesarCipher class with char encipher(char input) and char decipher(char input) methods that hides the HashMap from the input processing.

This has the added benefit that after you have done this, it becomes trivial to replace the HashMap with an efficient data structure or algorithm. It is a way to separate different responsibilities in your code to separate modules.