Caesar-Cipher - 2nd follow-up

Question

I've now used the tips, you can find here and there, to improve my code:

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

    enum WhatToDo {ENCRYPT, DECRYPT};

    public static void main(String[] args) {

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

        String message = "Please enter shift to the right:";
        int shift = findShift(message);

        String out = "";
        message = "Encrypt or decrypt?";

        out = MakeDecision(message, text, shift);

        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);
    }

    public static int findShift(String msg) {
        String UserInput = JOptionPane.showInputDialog(msg);
        int shift;
        try {
           shift = Integer.parseInt(UserInput);
        }
        catch (NumberFormatException e) {
            shift = findShift("Please enter shift as a number:");
        }

        return shift;
    }

    public static String MakeDecision(String message, String text, int shift) {

        String UserInput = JOptionPane.showInputDialog(message);
        UserInput = UserInput.toUpperCase();
        String out = "";


        if(WhatToDo.ENCRYPT.name().equals(UserInput) == true) {
        boolean test = true;
        out = EncryptOrDecrypt(text, shift, test);
        }

        else if(WhatToDo.DECRYPT.name().equals(UserInput) == true) {
            boolean test = false;
            out = EncryptOrDecrypt(text, shift, test);

        }
        else {
            message = "Illegal Choice! Encrypt or decrypt?";
            out = MakeDecision(message, text, shift);
        }

        return out;

    }

    //Encryption
    public static String EncryptOrDecrypt(String text, int n, boolean test) {
        int count = 0;
        int alphabetLength = 0;
        int decryptFactor = 1;

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

        StringBuilder out = new StringBuilder(); //Empty string for result.
        while (count < text.length()) {
            final char currentChar = text.charAt(count);
            if (currentChar >= 'A' && currentChar <= 'Z') {
                if (currentChar + n > 'Z') {
                    alphabetLength = 26;
                }
                out.append((char)(currentChar + (n*decryptFactor) - (alphabetLength*decryptFactor)));
            } 
            else {
                out.append(currentChar);        
            }
            count++;
            alphabetLength = 0;
        }
        return out.toString();
    }

}

Do you think that this now is good code? Do you have any tips for further improvement?

Answer 1

A few things about your encryption algorithm:

I don't think you gain anything by using a StringBuilder. Modifying a char[] is easier to use and just as easy to make a string with.

Your enum I would suggest naming it something like EncryptionMode.

The names of the arguments need work. I would suggest instead of n use shift, instead of test change it to take the enum(EncryptionMode mode)

You're restricting the shift to just the upper case alphabet. To me it makes more sense to use the whole ASCII character set. This way you can handle any letters, numbers and punctuation.

Instead of assigning the shift modifier to a separate variable, I would suggest modifying the shift variable.

Anytime you use magic values try to set up constants instead. This puts names to the values, making it easier to decipher why they are there.

Putting this all together. It could look like this:

final static char UPPER_LIMIT = (char)255;
final static int NO_ASCII_CHARS = 256;
enum EncryptionMode {ENCRYPT, DECRYPT};
public static String EncryptOrDecrypt(String text, int shift, EncryptionMode mode) {

    if(mode == EncryptionMode.DECRYPT) {
        shift *= -1;
    }
    char[] chars = text.toCharArray();
    for(int i = 0; i < chars.length; ++i){
        chars[i] += shift;
        if(chars[i] < '\0'){
            chars[i] = (char)(chars[i] + UPPER_LIMIT);
        }
        else{
            chars[i] = (char)(chars[i] % NO_ASCII_CHARS );
        }
    }
    return new String(chars);
}

Took another look and realized this code could be more performant. Here's the revision:

public static String EncryptOrDecrypt(String text, int shift, EncryptionMode mode) {

    if(mode == EncryptionMode.DECRYPT) {
        shift *= -1;
    }
    char[] chars = new char[text.length()];
    for(int i = 0; i < chars.length; ++i){            
        chars[i] = (char)(shift + text.charAt(i));
        if(chars[i] < '\0'){
            chars[i] = (char)(chars[i] + UPPER_LIMIT);
        }
        else{
            chars[i] = (char)(chars[i] % NO_ASCII_CHARS );
        }
    }
    return new String(chars);
}  

Answer 2

I'm going to copy and paste snippets from your other reviews that you havne't implemented as well as my own review:

shortened names like out or d don't really have any benefit over slightly longer, clearer ones

You still use the exact name "out" in your code:

String out = "";

stick with code style guides

Java in particular has common naming standards. You should use lowerCamelCase for class & method variables such as:

String UserInput

You may even notice StackExchange has given it different highlighting. Normally UpperCamelCase is used for classes.

while (count < ...

I'd suggest using a for-each loop here instead since you never use the 'count' variable

WhatToDo 

I really don't like this name. I missed it at the top and it really surprised me when I first saw it used. "WhatToDo" Is not a good name.

"test" is also a really bad name. There are lots of information available online & on this site about variable namings. As a general rule, try to look at the name by itself and see if it's at all descriptive.

I don't see the point in declaring the variable "test". Just pass a boolean true/false directly to the method.