follow-up - caesar-cipher - java

Question

I now processed the suggestions you can find here. I've deleted some duplications and corrected some other minor mistakes.

Here's the code:

import javax.swing.JOptionPane;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import java.awt.Dimension;
public class caesar {

    public static void main(String[] args) {
        String field, text;

        field = JOptionPane.showInputDialog("Please enter text:");
        field = field.replaceAll("[^a-zA-Z]+", "");
        field = field.toUpperCase();

        int shift;

        String shift_String = JOptionPane.showInputDialog("Please enter shift to the right:");
        shift = Integer.parseInt(shift_String);

        String d = JOptionPane.showInputDialog("Encrypt (1) or decrypt (2):");
        int decision = Integer.parseInt(d);

        String out = "";


        if(decision == 1) {
            boolean test = true;
            out = operation(field, shift, test);

        }
        else if(decision == 2) {
            boolean test = false;
            out = operation(field, shift, test);

        }
        else if(decision != 1 && decision != 2) {
            out = "Invalid choice!";
        }
        JTextArea msg = new JTextArea(out);
        msg.setLineWrap(true);
        msg.setWrapStyleWord(true);
        JScrollPane scrollPane = new JScrollPane(msg);
        scrollPane.setPreferredSize(new Dimension(300,300));
        JOptionPane.showMessageDialog(null, scrollPane);
    }

    //Encryption
    public static String operation(String text, int n, boolean test) {
        int x = 0;
        int y = 0;
        int z = 1;

        if(!test) {
            z = -1;
        }

        StringBuilder out = new StringBuilder(); //Empty string for result.
        while (x < text.length()) {
            final char currentChar = text.charAt(x);
            if (currentChar > 64 && currentChar < 91) {
                if (currentChar + n > 90) {
                    y = 26;
                }
                out.append((char)(currentChar + (n*z) - (y*z)));
            } 
            else {
                out.append(currentChar);        
            }
            x++;
            y = 0;
        }
        return out.toString();
    }

}

Are there anymore suggestions to improve the code?

Answer 1

Naming

Give your class a more meaningful name, that begins with a capital letter -as all classes should in Java-, e.g. CaesarCipher.

The operation() method's name doesn't tell us anything about what it actually does. If it weren't for the //Encryption comment, there would be no indication that this class does something even related to encryption/decryption.

The variable shift_String uses snake_case while all the other ones use camelCase; pick one and stick to it (and since this is Java, I'd recommend camelCase as it is the accepted standard).

More generally, try using names that actually describe what the variable/method/class represents or does (what is d? what about test?). You could for example declare one String, say userInput, and reuse it whenever you want to store the user's input, that way you don't need to declare two variables where one has the same name as the other appended with String everytime.

Also, shortened names like out or d don't really have any benefit over slightly longer, clearer ones, other than making the very first time you type the name a tiny bit shorter. Giving them more meaningful names will make the code easier to read later, and you shouldn't be worried about typing a few more characters each time; let your IDE do its job!

final keyword

This recommendation is more based on my own opinions, but I think it's justified here since you've made use of it yourself.

Mark all method arguments as final; this will prevent you from accidentally reassigning them (which you really shouldn't do anyways).

Make all your instance & local variables final when appropriate; this simply indicates that once they are assigned a value, that value should not and will not change in the future. It may seem like you're simply adding more unnecessary fluff to an already very verbose language, but I personally think it helps writing clearer code as well as making it easier to debug.

If you want to read more about Java's use of the final keyword, I recommend this answer : https://softwareengineering.stackexchange.com/questions/98691/excessive-use-final-keyword-in-java.

Code Optimization & Reusability

The text variable is declared but never used.

Redundant checks in main() :

if(decision == 1) {
    boolean test = true;
    out = operation(field, shift, test);
}
else if(decision == 2) {
    boolean test = false;
    out = operation(field, shift, test);
}
else if(decision != 1 && decision != 2) {
    out = "Invalid choice!";
}

Your last else if could be replaced by a simple else, since the only way you'll get there is if decision is neither equal to 1 nor to 2. In this same snippet, you could also get rid of the test variable altogether, or maybe declare it before those conditionals and place the out = operation(field, shift, test); at the end. Additionnaly, what does test mean? What is it testing for?

You may want to implement another cipher later; if that's the case, then creating a simple Cipher interface might be worthwhile. You should also make caesar a separate class from main() and decouple it from all the user interface stuff, that way you can use it whenever you want, wherever you want. The interface might look something like this :

public interface Cipher {
    public String encrypt(final String text);

    public String decrypt(final String cipherText);
}

Use an enum to represent the two choices given to the user. This might be overkill for this case as you'd only have two possible values (ENCRYPT and DECRYPT, for example), but it would certainly make the code even more readable.

There's more that could be said, about user input validation, or using magic numbers in operation(), and more, but I'll leave it to other users.

Exception Handling & User Input

What happens if the user decides to type "oiuahffduht$$$" when you ask them for the shift value? Your program breaks and the user has to manually start it again! Always assume the worst from the user. Ideally, you'd want to handle such cases in your code via exception handling. If you're not familiar with the concept, you can take a look at this link which covers all the basics of EH in Java : https://stackify.com/specify-handle-exceptions-java/.

So what can we do here? If you look at the Java documentation for Integer.parseInt() -or check its signature from your favorite IDE, you'll see that it can throw a NumberFormatException, so let's catch it. Keeping the same logic and syntax as your original code, it might look something like this :

String shift_String = JOptionPane.showInputDialog("Please enter shift to the right:");

try {
    shift = Integer.parseInt(shift_String);
}
catch (NumberFormatException e) {
    // Handle the exception
}

There's lots of different ways you can handle the exception; you could simply show the user the contents of the exception or the stack trace, you could end the program... Personally, I think the best way to handle this case would be to show a new dialog that says something along the lines of "Error, you must enter a number", then show that previous input box and asks for the same thing again. That way the user won't have to close/reopen the program and they'll know what went wrong.

Magic Numbers

In your operation() method, you use a bunch of "magic numbers" (64, 90, 26...). So what's a magic number? It's basically a hardcoded value (not necessarily a 'number') that is present in your code without any explanation of why it's there or what it means.

Imagine if someone who's not familiar with ASCII were to read your code (though maybe they shouldn't be writing code in the first place). They might think you're using some crazy voodo magic to turn numbers into letters, who knows? So give these numbers names!

Or, maybe try a different approach entirely, where you would store all the possible characters in a String, then make use of the wonderful modulo operator like so :

private static final String ALPHABET = "abcdefghijklmnopqrstuvwxyz";

public static String encrypt(final String text, final int shift) {
        final String lowerCaseText = text.toLowerCase();
        String cipherText = "";

        for (int i = 0; i < lowerCaseText.length(); i++) {
            final int charPosition = ALPHABET.indexOf(lowerCaseText.charAt(i));
            final int newCharPosition = (shift + charPosition) % 26;
            final char newChar = ALPHABET.charAt(newCharPosition);

            cipherText += newChar;
        }

        return cipherText;
    }

Now you don't even have to deal with those annoying numbers anymore!

By the way, I haven't tested this, and I don't claim it to be more efficient or readable.