2
\$\begingroup\$

I already asked for help in optimizing a simple Caesar Cypher code in java here : Cesar code in Java I tried to implement all the advice from that subject and now I am asking again. Do you have any other advice to optimize this code ? What can be improved ?

Here is the code, thank you in advance :

Cypher.java :

package fr.thomascloarecd.cypher;

import java.util.Scanner;

public class Cypher {

    private static Scanner sc = new Scanner(System.in);

    private static String cypherMethod = "";

    public static void main (String[] args){
        while(!(cypherMethod.equals("stop") || cypherMethod.equals("2"))) {
            System.out.println("Choose a cypher's method and type it :" +
                    "\n (1) - caesarCypher" +
                    "\n (2) - stop => To stop the program");

            cypherMethod = sc.nextLine().toLowerCase();

            if (cypherMethod.equals("1") || cypherMethod.equals("caesarcypher")) {

                String encryptOrDecrypt = "";
                while(!(encryptOrDecrypt.equals("back") || encryptOrDecrypt.equals("3"))) {
                    System.out.println("\nWhat do you want to do (type it) :" +
                            "\n (1) - encrypt" +
                            "\n (2) - decrypt" +
                            "\n (3) - back => To go back in the cypher's method summary");

                    encryptOrDecrypt = sc.nextLine().toLowerCase();

                    if (encryptOrDecrypt.equals("1") || encryptOrDecrypt.equals("encrypt")) {
                        System.out.println("\nThen, what's the sentence that you want to encrypt ?");

                        CaesarCypher caesarcypher = new CaesarCypher(sc.nextLine(), encryptOrDecrypt);

                        System.out.println("\nEncrypted with caesar cypher : "+caesarcypher.translatedSentence());

                    } else if (encryptOrDecrypt.equals("2") || encryptOrDecrypt.equals("decrypt")) {
                        System.out.println("\nThen, what's the sentence that you want to decrypt ?");

                        CaesarCypher caesarcypher = new CaesarCypher(sc.nextLine(), encryptOrDecrypt);

                        System.out.println("\nDecrypted with caesar cypher : "+caesarcypher.translatedSentence());
                    }
                }
            } else if (cypherMethod.equals("2") || cypherMethod.equals("stop")) {
                System.exit(0);
            }
        }
        sc.close();
    }
}

CaesarCypher.java :

package fr.thomascloarecd.cypher;

import java.text.Normalizer;

class CaesarCypher {
    private String userSentence;
    private String encryptOrDecrypt;

    CaesarCypher(String pUserSentence, String pEncryptOrDecrypt){
        userSentence = Normalizer.normalize(pUserSentence, Normalizer.Form.NFD).replaceAll("[^\\p{ASCII}]", "").toLowerCase();
        encryptOrDecrypt = Normalizer.normalize(pEncryptOrDecrypt, Normalizer.Form.NFD).replaceAll("[^\\p{ASCII}]", "").toLowerCase();
    }

    String translatedSentence(){
        char[] ALPHABET = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'};

        StringBuilder translatedSentence = new StringBuilder();

        for(int i = 0; i <= userSentence.length()-1; i++){
            for(int j = 0; j < ALPHABET.length; j++) {
                if (userSentence.charAt(i) == ALPHABET[j]) {
                    translatedSentence.append(ALPHABET[(26 + j + integerDependOnAction()) % 26]);
                    break;
                } else if (j == 25) {
                    translatedSentence.append(userSentence.charAt(i));
                    break;
                }
            }
        }
        return translatedSentence.toString();
    }

    private int integerDependOnAction() {
        if (encryptOrDecrypt.equals("encrypt") || encryptOrDecrypt.equals("1")) {
            return 1;
        } else if (encryptOrDecrypt.equals("decrypt") || encryptOrDecrypt.equals("2")) {
            return -1;
        }
        return 0;
    }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

My first observation was - without actually reading anything - WOW that's a lot of code for such a simple task.

Then after the first read-through I was confused because I couldn't say if this was due to over-engineering or under-engineering.

It looks over-engineered because it includes a lot of (possibly unnecessary?) functionality (things like a "huge" menu system and Unicode normalization) just for demonstrating Caesar's. OTOH it looks under-engineered, because there is a lot of very verbose code and repetition.


I'll concentrate on the actually encryption class first.

A glaring problem here, is that you are missing the point of object-oriented programming. Having a class that you just instantiate to do one thing and then destroy it again isn't really the point of OOP.

It would be more sensible to have a class that, once instantiated, is used multiple times. Something like this:

CaesarCypher cypher = new CaesarCypher();

System.out.println(cyhper.encrypt("Example"));
System.out.println(cyhper.decrypt("Fybnqmf"));

The next problem would be the interface. Passing the information if you want to encrypt or decrypt as a String is very problematic, especially since those strings are tightly coupled to your menu.

Imagine this were a real production encryption system: The menu is changed in some way (for example, menu items are renumbered or translated), but the developer doing it doesn't know that requires changing the encryption class, or there is simply a small typo in one of the strings, then there will be no way to know something is wrong. The class doesn't report any errors, instead it just sends out everything unencrypted, which is potentially catastrophic.

Instead you should be using a parameter that only allows two possible values to represent encryption and decryption, for example a boolean or an enumeration:

enum CyperDirection { ENCRYPT, DECRYPT; }

Or use two separate methods as I did in the example above.

And even is you do use a String, then you shouldn't be calculating the "integerDependOnAction" inside the loop, but just once at the start.


The ALPHABET variable should actually be a constant (a final static field) in the class.


On to the actual algorithm:

A StringBuilder isn't really needed here. Internally a StringBuilder is just a char array, that is "automatically" extended if needed. But since you know how long the resulting string will be you don't need the automatic extending, so either at least initialize the StringBuilder with a capacity (new StringBuilder(userSentence.length())) or (because you are working character by character anyway) just use a char array of the correct length.

Your outer loop would be nicer as for (int i = 0; i < userSentence.length(); i++) { - just as you did in the inner loop. And don't forget proper formatting: a space after the for keyword and before the opening bracket.

Inside the loop you have a bug: the condition j == 25 can never be true.

In the end the class could look something like this:

class CaesarCypher {
    private static final char[] ALPHABET = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'};

    private int shift;

    // This is just an example, why not to use a "one time" class: You can 
    // configure it - in this case by allowing a different size shift.
    public CaesarCypher(int shift) {
        if (shift <= 0) {
            throw new IllegalArgumentException("shift must be a positive integer greater than zero.");
        }
        this.shift = shift;
    }

    public CaesarCypher() {
        this(1);
    }

    public String encrypt(String text) {
        return translate(text, this.shift);
    }

    public String decrypt(String text) {
        return translate(text, -this.shift);
    }

    private String normalize(String text) {
        return Normalizer.normalize(text, Normalizer.Form.NFD).replaceAll("[^\\p{ASCII}]", "").toLowerCase();
    }

    private String translate(String text, int shiftBy) {

        String normalizedText = normalize(text);

        char[] result = new char[normalizedText.length()];

        for (int i = 0; i < normalizedText.length(); i++) {
            for (int j = 0; j < ALPHABET.length; j++) {
                if (normalizedText.charAt(i) == ALPHABET[j]) {
                    result[i] = ALPHABET[(j + shiftBy) % ALPHABET.length];
                    break;
                } 
            }
            // 0 is the default value for char. If the current element of the 
            // result array is still zero, then original character wasn't in the 
            // ALPHABET. In this case just use the original character.
            if (result[i] == 0) {
                result[i] = normalizedText.charAt(i);
            }
        }

        return new String(result);
    }
}

Finally a few short words the menu system: The nested ifs and long conditions ((cypherMethod.equals("1") || cypherMethod.equals("caesarcypher"))) makes the code very unreadable.

If the goal of this exercise was to show case the Caesar's cypher, then you may want to "cut back" on the menu system and use tests to demonstrate your code.

But if you consider the menu part of the exercise then you should consider maybe building a "proper" small framework to make the menu system more readable and maintainable.

\$\endgroup\$
2
  • \$\begingroup\$ Hello, what do you mean by a "proper" small framework ? Something like a little UI with JFX ? \$\endgroup\$ Dec 12, 2018 at 6:31
  • 1
    \$\begingroup\$ @ThomasCloarec No, I exaggerated with the word "framework". Just a method or a class to encapsulate the menu(s). \$\endgroup\$
    – RoToRa
    Dec 14, 2018 at 0:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.