8
\$\begingroup\$

I am a recent beginner in Java and have attempted a simple Caesar cipher (using only lowercase):

public class RunEncryption
{
    public static void main(String[] args)
    {
        SecretMessage test1 = new SecretMessage();
        test1.setMessage("hello");
        test1.display();
        test1.encrypt(1);
    }

}

This is my class with the main method in it:

public class SecretMessage
{
    String message;

    void setMessage(String newMessage) {
        message=newMessage;
    }

    void display() {
        System.out.println(message);
    }

    void encrypt(int key) {
        int len = message.length();
        for(int i=0; i<len; i++) {
            char c = message.charAt(i);
            c = (char)(c+key);
            System.out.print(c);
        }
    }

}

This works thankfully. I will get to wrapping the letters around. But I want to ask if there is anything maybe unconventional about this or if it is inefficient code. Any help would be appreciated as I am still a beginner. Also, how long does it take to make this code? It took me far too long.

\$\endgroup\$
  • \$\begingroup\$ encrypt should return a String, not just print chars. \$\endgroup\$ – Eric Duminil Feb 4 '18 at 10:15
  • 1
    \$\begingroup\$ There's a bug left: try encrypt("zebra"). \$\endgroup\$ – Roland Illig Feb 4 '18 at 13:17
6
\$\begingroup\$

Nothing unusual about your code, except for the visibility of the message which would usually be private. Your encrypt method is a bit off, as it is just printing while encrypting, maybe you could call it printEncrypted or create a new method that just encrypts and saves the results in a second variable..

You could take a less imperative approach as well, this is a rewrite of your method to do the same thing, a little bit more declarative.

void encrypt( int key ) {
        message.chars( )
               .map( c -> c + key )
               .forEach( c -> System.out.print( ( char ) c ) );
}

from: https://stackoverflow.com/questions/48601918/simple-caesar-cipher-java/48602653#48602653

\$\endgroup\$
7
\$\begingroup\$

IMHO separating computation and printing is the most important improvement to do.

  • A code which can't do the computation separately is just a throw-away junk; you can't ever use it for anything else.
  • A code which can't do the printing separately is useless for the same reason. You can't use it for anything else, not even for the decryption.

In s simple snippet it doesn't matter much, but learning it later with complicated algorithms is much harder.


Note an asymmetry: Your SecretMessage stores the plaintext, but not the ciphertext. Ask yourself, if it needs to store anything.

Think about naming. Is the SecretMessage really a message? Or is it maybe an Encryptor? The way you implemented it, your naming isn't bad, but when you change it, you should reconsider it.

Does encrypt really encrypt its argument? Or would you expect it to do something else? Maybe being more verbose with encryptWithKey(char key) makes more sense.


Efficiency is usually overrated. You must think about using efficient algorithms and data structures for the job, but you can ignore nearly everything else. Anyway, before you start optimizing, make sure you optimize the real bottleneck, i.e., profile you code.

Anyway, using System.out.print for single characters may slow you down as it's synchronized and it flushes the output.

First building the result (using StringBuilder) would be faster. Moreover, it's a step in the right direction (Separation of concerns).

\$\endgroup\$
  • \$\begingroup\$ To add on the naming of the class another more describing name could be CaesarEncryptor. This would make its use-case even more apparent. \$\endgroup\$ – AnotherGuy Feb 4 '18 at 1:00
  • \$\begingroup\$ I agree, I would expect an encrypt function to return a string (encrypted of course) that could be used somewhere else (e.g. in a CommunicationsFunction or a PrintFunction). \$\endgroup\$ – AJD Feb 4 '18 at 4:56

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.