3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ In EncryptOrDecrypt, why is the if (currentChar >= 'A' && currentChar <= 'Z') { test there? Aren't all non-alphabetics filtered out and everything set to uppercase? \$\endgroup\$ – JollyJoker Dec 13 '19 at 12:20
  • 1
    \$\begingroup\$ That's true. But I just ask myself, whether this makes sense. I mean, I want to just encrypt alphabetic symbols, but it doesnt really makes sense to filter it out. I will change this and only filter spaces. \$\endgroup\$ – chrysaetos99 Dec 13 '19 at 12:30
5
\$\begingroup\$

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);
}  
\$\endgroup\$
  • \$\begingroup\$ I disagree on using arrays. What do you think StringBuilder is for if you wouldn't use it here? \$\endgroup\$ – JollyJoker Dec 13 '19 at 11:06
  • \$\begingroup\$ My take is it works best for strings that will have an unknown length. If the length is predetermined it is much simpler and easier to use a char array. \$\endgroup\$ – tinstaafl Dec 13 '19 at 17:57
  • \$\begingroup\$ I try to avoid referring to anything by indexes unless I have to, which maybe explains why I see this differently. I'd use for(char c : text.toCharArray()) with a StringBuffer or just do it via an IntStream \$\endgroup\$ – JollyJoker Dec 13 '19 at 20:47
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I've changed "out" to "output". Also I now use UpperCamelCase for classes and methods and lowerCamelCase for variables. Also, I renamed "WhatToDo". It's now called "encryptionOrDecryption". Anything else I should think about? \$\endgroup\$ – chrysaetos99 Dec 12 '19 at 19:41
  • \$\begingroup\$ @chrysaetos99 There is more, my review was pretty quick. methods are normally lowerCamelCase. \$\endgroup\$ – dustytrash Dec 12 '19 at 19:49
  • \$\begingroup\$ Oh ok, then I will change this. Could you go a bit into detail what your other suggestions are? \$\endgroup\$ – chrysaetos99 Dec 12 '19 at 20:01

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.