8
\$\begingroup\$

I've wrote a little program that encrypts text by using the caesar-cipher. Also it contains a little GUI, created by using swing. Here's the full 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) {
            out = encrypt(field, 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);
        }
        if(decision==2) {
            out = decrypt(field, 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);
        }
    }

    //Encryption
    public static String encrypt(String text, int n) {
        int x = 0;
        int y = 0;
        String out = ""; //Empty string for result.
        while (x < text.length()) {
            if (text.charAt(x) > 64 && text.charAt(x) < 91) {
                if (text.charAt(x) + n > 90) {
                    y = 26;
                }
                out = out + (char) (text.charAt(x) + n - y);
            } else {
                out = out + text.charAt(x);
            }
            x++;
            y = 0;
        }
        return out;
    }

    //Decryption
    public static String decrypt(String text, int n) {
        int x = 0;
        int y = 0;
        String out = ""; //Empty string for result.
        while (x < text.length()) {
            if (text.charAt(x) > 64 && text.charAt(x) < 91) {
                if (text.charAt(x)-n < 65) {
                    y = 26;
                }
                out = out + (char) (text.charAt(x) - n + y);
            } else {
                out = out + text.charAt(x);
            }
            x++;
            y = 0;
        }
        return out;
    }
}

My question now is: How to improve this code?

I mean, it does what it is supposed to do, but it's not really great code.

\$\endgroup\$
  • 3
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Feel free to post a follow-up question if the code has changed significantly enough. \$\endgroup\$ – brug Dec 10 '19 at 18:44
  • \$\begingroup\$ Oh, I am sorry, thanks for changing it for me. \$\endgroup\$ – chrysaetos99 Dec 10 '19 at 19:09
  • \$\begingroup\$ Thanks for the hint. You can find the follow up question at: codereview.stackexchange.com/questions/233811/… \$\endgroup\$ – chrysaetos99 Dec 10 '19 at 19:18
6
\$\begingroup\$

In my opinion, the main problem with your code is the duplication, here is my advices.

1) Put the ui code out of the conditions.

The only issue there, if the choice is invalid, you can either show a default string, or throw an exception.

        if (decision == 1) {
            out = encrypt(field, shift);
        } else if (decision == 2) {
            out = decrypt(field, shift);
        } else {
            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);

Or

        if (decision == 1) {
            out = encrypt(field, shift);
        } else if (decision == 2) {
            out = decrypt(field, shift);
        } else {
            throw new IllegalStateException("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);

2) In the encrypt & decrypt, to create the string containing the result, i suggest that you use java.lang.StringBuilder instead of concatening the String; you will gain some performance.

    public static String decrypt(String text, int n) {
        int x = 0;
        int y = 0;
        StringBuilder out = new StringBuilder(); //Empty string for result.
        while (x < text.length()) {
            if (text.charAt(x) > 64 && text.charAt(x) < 91) {
                if (text.charAt(x) - n < 65) {
                    y = 26;
                }
                out.append(text.charAt(x) - n + y);
            } else {
                out.append(text.charAt(x));
            }
            x++;
            y = 0;
        }
        return out.toString();
    }

3) In the encrypt & decrypt, extract the text.charAt(x) in a variable, to remove the duplicates.

    public static String decrypt(String text, int n) {
        int x = 0;
        int y = 0;
        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 < 65) {
                    y = 26;
                }
                out.append(currentChar - n + y);
            } else {
                out.append(currentChar);
            }
            x++;
            y = 0;
        }
        return out.toString();
    }

4) The encrypt and decrypt methods are pretty similar, you can probably merge them if you want.

    //Encryption
    public static String encrypt(String text, int n) {
        return operation(text, n, true);
    }

    //Decryption
    public static String decrypt(String text, int n) {
        return operation(text, n, false);
    }

    public static String operation(String text, int n, boolean isEncryption) {
        int x = 0;
        int y = 0;
        StringBuilder out = new StringBuilder(); //Empty string for result.
        while (x < text.length()) {
            final char currentChar = text.charAt(x);

            if (currentChar > 64 && currentChar < 91) {
                if (isEncryption ? (currentChar + n > 90) : (currentChar - n < 65)) {
                    y = 26;
                }
                out.append(isEncryption ? (currentChar + n - y) : (currentChar - n + y));
            } else {
                out.append(currentChar);
            }
            x++;
            y = 0;
        }
        return out.toString();
    }
```
\$\endgroup\$
  • \$\begingroup\$ Thank you very much. That really helps. Do you have an idea how to merge the both methods? \$\endgroup\$ – chrysaetos99 Dec 10 '19 at 18:04
  • \$\begingroup\$ Sorry, I forgot the code snippet in my answer, I updated it. \$\endgroup\$ – Doi9t Dec 10 '19 at 18:14
  • \$\begingroup\$ Ah, thanks, that helped! \$\endgroup\$ – chrysaetos99 Dec 10 '19 at 18:37
4
\$\begingroup\$

something in addition:

  • stick with code style guides. (e.g. I prefer: https://google.github.io/styleguide/javaguide.html .Something got in my eye: public class caesar, shift_String)
  • validate users input and notify if something is wrong (it would more user friendly and safe)
  • assign 'magic' numbers to a constants (in encrypt/decrypt methods, will add more readability to the code)
  • avoid code duplication (encrypt/decrypt and decision bodies)

an example of how main could look like:

public static void main(String[] args) {
    String inputText = JOptionPane.showInputDialog("Please enter text:");
    String normalizedInput = normalizeText(inputText);

    int shiftBy = getIntFromInput("Please enter shift to the right:");
    int option = getIntFromInput("Encrypt (1) or decrypt (2):"); // todo for options i'd recommend to use Enum

    // todo arg validation example
    String resultMessage;
    switch (option) {
        case 1:
            resultMessage = encrypt(normalizedInput, shiftBy);
            break;
        case 2:
            resultMessage = decrypt(normalizedInput, shiftBy);
            break;
        default:
            resultMessage = "Unsupported option: " + option;
    }
    showDialogWithMessage(resultMessage);
}

private static String normalizeText(String inputText) {
    return inputText
            .replaceAll("[^a-zA-Z]+", "")
            .toUpperCase();
}

private static int getIntFromInput(String message) {
    return Integer.parseInt(JOptionPane.showInputDialog(message));
}

private static void showDialogWithMessage(String message) {
    JTextArea msg = new JTextArea(message);
    msg.setLineWrap(true);
    msg.setWrapStyleWord(true);
    JScrollPane scrollPane = new JScrollPane(msg);
    scrollPane.setPreferredSize(new Dimension(300, 300));
    JOptionPane.showMessageDialog(null, scrollPane);
}
\$\endgroup\$

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.