1
\$\begingroup\$

I am coding in java since some time but I was not always really organized about methods and commentaries, here is a quick code to use the Cesar code :

package cesar_code;

import java.text.Normalizer;
import java.util.Scanner;

public class CesarCode {

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

    private static boolean stop = false;
    private static boolean ready_to_translate = false;

    private static String user_input;
    private static String message_head;

    public static void main (String[] args){
        while(!stop){
            choose_action_to_do();
            if(ready_to_translate){
                translate();
            }
        }
    }

    private static void choose_action_to_do(){
        ready_to_translate = false;
        System.out.println("\n\nType \"encrypt(message_to_encrypt)\" or \"decrypt(message_to_decrypt)\" please (or \"stop\" to stop).");

        user_input = sc.nextLine();
        user_input = (Normalizer.normalize(user_input, Normalizer.Form.NFD).replaceAll("[^\\p{ASCII}]", "")).toLowerCase();

        if(user_input.equals("stop")){
            stop = true;
        }
        else if (user_input.length()>= 9){
            message_head = user_input.substring(0,8);
            if ((message_head.equals("encrypt(") || message_head.equals("decrypt(")) && user_input.charAt(user_input.length() - 1) == ')') {
                ready_to_translate = true;
            }
        }
    }

    private static void translate(){
        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', 'x', 'z'};

        StringBuilder translated_message_build = new StringBuilder();
        System.out.print("The encrypted message is : ");
        for(int i = 8; i <= user_input.length()-2; i++) {
            for(int j = 0; j< alphabet.length; j++){
                if(user_input.charAt(i)==alphabet[j]){
                    if (message_head.equals("encrypt(")){
                        if(j!=26){
                            translated_message_build.append(alphabet[j+1]);
                        }
                        else{
                            translated_message_build.append((alphabet[j-26]));
                        }
                        break;
                    }
                    else if (message_head.equals("decrypt(")){
                        if(j!=0){
                            translated_message_build.append(alphabet[j-1]);
                        }
                        else{
                            translated_message_build.append((alphabet[j+26]));
                        }
                        break;
                    }
                }
                else if(j == 26){
                    translated_message_build.append(user_input.charAt(i));
                }
            }
            System.out.print(translated_message_build.charAt(i-8));

            try {
                Thread.sleep(100);
            }
            catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }
}

Maybe there is something to improve ? Maybe commentaries are required here ? Should I combine the encrypt and decrypt part in one just with a return method that change the + or the - depending if it is encrypt or decrypt that is asked ?

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review. Are there two "a"s in "caesar" where you are from? \$\endgroup\$
    – chicks
    Nov 26, 2018 at 22:38
  • 1
    \$\begingroup\$ In France, I use to tell it "Code Cesar", but it seems that here it is more about a "Caesar Cipher" as 200_sucess edited my post. But anyway, that's not the subject ^^. \$\endgroup\$ Nov 26, 2018 at 22:57

3 Answers 3

1
\$\begingroup\$

While your code do everything as intended, there are quite a few things that are wrong or non-standard.

In no particular order :

  • the package name should usually start with an internet domain name extension, such as fr or com
  • variable should not contains underscore, they are usually reserved to constants name so it should not be user_input but rather userInput
    Here is the convention from the oracle website : https://docs.oracle.com/javase/tutorial/java/nutsandbolts/variables.html
  • same goes for method name
  • there are some "magic numbers"... but more on that later
  • it's more readable to have operator for comparisons separated from their operands by space such as user_input.length() >= 9
  • for readability as well, avoid line that are too long and put one space behind comma
  • your methods are really hard to reuse : if you wanted to use your caesar cipher in some other place, you wouldn't manage because your cipher method mix a lot of differents things : it ciphers (as expected), it reads from standard input, it prints, it sleeps (why would it do that ?) and it knows that the user will write "encrypt" or "decrypt" You should have made a method like this 'String cipher(String input)' that'll only do one thing : return the ciphered string It is thus much easier to test and to reuse. If you are not convinced, I'd recommend trying to unit test your translate method... you'll see it's a real pain :)
  • what's the point of separating choose_action_to_do and translate ? on paper they may sound like two differents features, but the fact that they are linked by a global variable demonstrates they are in fact tighly coupled in your code
  • most static variables don't make much sense, and if you apply the two previous points it'll become highly apparent
  • let's say you want to have another cipher method (like a "One-time pad" for example), making an interface to implement various Cipher method make sense IMO

Here is a part of the solution using everything I recommended : (Please bear with the minimal javadoc :))

package fr.cipher;

import java.text.Normalizer;
import java.util.Scanner;

public class Launch {

    private static final String ENCRYPT = "encrypt(";
    private static final String DECRYPT = "decrypt(";
    private static final int HEAD_SIZE = 8;

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

    private static boolean stop = false;

    public static void main(final String[] args) {
        while (!stop) {
            askActionAndApply();
        }
    }

    public static void askActionAndApply() {
        Cipher c = new CaesarCipher();
        System.out.println("\n\nType \"encrypt(message_to_encrypt)\" or "
                + "\"decrypt(message_to_decrypt)\" please (or \"stop\" to stop).");

        String userInput = sc.nextLine();
        userInput = Normalizer.normalize(userInput, Normalizer.Form.NFD).replaceAll("[^\\p{ASCII}]", "").toLowerCase();

        if(userInput.equals("stop")) {
            stop = true;
        } else if (userInput.length() > HEAD_SIZE && userInput.endsWith(")")) {
            translate(c, userInput);
        }
    }

    private static void translate(final Cipher cipher, final String userInput) {
        String messageHead = userInput.substring(0, HEAD_SIZE);

        String content = userInput.substring(HEAD_SIZE, userInput.length() - 1);

        if (messageHead.equals(ENCRYPT)) {
            System.out.println(cipher.cipher(content));
        } else if (messageHead.equals(DECRYPT)) {
            System.out.println(cipher.decipher(content));
        }
        // if you want you can put your Thread.sleep here :)
    }
}

You could (and should) also remove the "stop" field but I let it in place so it's easier to compare your code and mine. The Cipher interface looks like this :

/**
 * This class represents a cryptography algorithm.
 * <p>
 * It is not suited for <i>Hash</i> algorithm.
 * 
 * @author R Dhellemmes
 *
 */
public interface Cipher {
    String cipher(String input);

    String decipher(String input);
}

And you'll have a CaesarCipher class implementing this Cipher interface.

Hope it helps :)

\$\endgroup\$
2
  • \$\begingroup\$ Thanks a lot guys ! Seems like i still got a lot to learn :P. As it's completely a new way to code for me, I know that I'll still have some questions after changing all my program. It could take me few days to understand everything exactly, so I'll be back right after for last questions ^^. \$\endgroup\$ Nov 28, 2018 at 10:23
  • \$\begingroup\$ @ThomasCloarec no problems :) if you want us to review your upgraded code, you should ask a brand new question ;) \$\endgroup\$ Nov 28, 2018 at 15:19
1
\$\begingroup\$

One thing that stands out for me in your code is the lack of use of the remainder %. i%n gives you the remainder of the Euclidean division of n by i, which is a positive integer >=0 and < n.

For example, 5%12 = 5 because 5 = 12*0 + 5, and 13%12 = 1 because 13 = 12*1 + 1 (I am using n=12 in this example because this is basically what we do when reading clocks).

Anyways, this is useful if you have a list of things which is "cyclic", i.e. you would like to go back to the beginning after reaching the end, which is exactly the case here.

So, for encryption, instead of

 if(j!=26){
      translated_message_build.append(alphabet[j+1]);
      }
 else{
      translated_message_build.append((alphabet[j-26]));
      }

I would write

translated_message_build.append(alphabet[(j+1)%26]);

And for decryption

translated_message_build.append(alphabet[(j-1)%26]);

This works because (for encryption) if j=26, j+1 = 27 and 27%26 = 1 because 27=26*1 + 1.

And (for decryption) if j=0, j-1 = -1 and -1%26 = 25 because -1 = 26*(-1) +25.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot guys ! Seems like i still got a lot to learn :P. As it's completely a new way to code for me, I know that I'll still have some questions after changing all my program. It could take me few days to understand everything exactly, so I'll be back right after for last questions ^^. \$\endgroup\$ Nov 28, 2018 at 10:23
0
\$\begingroup\$

As said in the other answers there is room to improvement. First of all you should apply the naming conventions to your package, fields and methods. Then you may want to separate the concerns of asking the user and doing the translation so that you can easily unit test your encoding and decoding algorithms.

Managing user interactions

The user interactions will be done by a dedicate class that, as you have done, continuously ask the user for an action and execute it. But if you want to use the "functional" constructs of Java8 you can see that actions as something that change the state of your application by setting the stop flag to true or by translating the user input.

public void start() {   
    do {
        Consumer<EncoderUi> action = askForAction();
        action.accept(this);
    } while (!stop);
}


private Consumer<EncoderUi> askForAction(){
    System.out.println("\n\nType \"encrypt(message_to_encrypt)\" or \"decrypt(message_to_decrypt)\" please (or \"stop\" to stop).");
    final String input = Normalizer.normalize(sc.nextLine(), Normalizer.Form.NFD)
            .replaceAll("[^\\p{ASCII}]", "")
            .toLowerCase();

    if ( "stop".equals(input) ) {
        return ui -> ui.stop = true;
    } else if ( input.startsWith("encrypt(") ) {
        return translate(input, cipher::encode);
    } else if ( input.startsWith("decrypt(") ) {
        return translate(input, cipher::decode);
    } else {
        return ui -> ui.print("Invalid action \""+input+"\".");
    }
}

Your encrypt or decrypt actions are basically doing the same thing :

  1. Extract the message from the user input
  2. Transform the message
  3. Print the result

The transformation is basically a Function<String, String> that takes a string and return another. And this is the only thing that changes in your process. You can use the method references to pass the correct transformation and keep the whole process the same.

private Consumer<EncoderUi> translate(String input, Function<String, String> algorithm) {
    return ui -> {
        String message = parse(input);
        String result = algorithm.apply(message);
        ui.print(result);
    };
}

You cannot unit test your user interactions but the code is quite clear so this is not a real issue. However if you want to do it, you can move the parsing of the user input to another method (or class) and test that each input produce an action that change your state as expected

MockedUi state = // ...
Consumer<EncoderUi> action = parse("stop");
action.accept(state);

assertThat(state.stop).isTrue();

Improving your translation

As said in the other answers your translation code can also be refactored to something more reusable. As previously said the encoding and decoding are basically a transformation of string with the only variation being the direction of the shift. The only difference is the way you compute the substitution character.

public String encode(String input) {
    return translate(input, chr -> chr+1);
}

public String decode(String input) {
        return translate(input, chr -> chr-1);
    }

private String translate(String input, IntUnaryOperator translation) {
    input.chars()
      .map(chr -> translation.apply(chr-'a'))
      .map(pos -> ALPHABET[pos % 26])
      .mapToObj(Objects::toString)
      .collect(joining());
}

You can avoid the nested loops by using the char[] ALPHABET = {'a', 'b',..} array. The chr-'a' is used to place to translate the Ascii char code to your array ('a' being at position 0).

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot guys ! Seems like i still got a lot to learn :P. As it's completely a new way to code for me, I know that I'll still have some questions after changing all my program. It could take me few days to understand everything exactly, so I'll be back right after for last questions ^^. \$\endgroup\$ Nov 28, 2018 at 10:23

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.