5
\$\begingroup\$

I've recently begun to teach myself to code. One simple project that I wanted to do was create a Caesar cipher that works well. Now that I feel I've accomplished that task, I'd like to know how I'm doing with overall code quality. Here's the code.

import java.util.Scanner;

public class Encryptor 
{
    private char[] upperCase = {'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 char[] lowerCase = {'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 char oneChar;
    private String encryptedString = "";
    private int counter;

    public String encryptString(String input, int offset)
    {
        for (counter = 0; counter < input.length(); counter++)
        {
            oneChar = input.charAt(counter);

            boolean moveOn = false;
            int i = 0;

            //get a single character

            while (!moveOn)
            {
                if (i > 26)
                {
                    encryptedString += oneChar;
                    moveOn = true;
                }
                //remainder keeps me from going over array length
                else if (oneChar == upperCase[i%26])
                {
                    encryptedString += upperCase[(i+offset)%26];
                    moveOn = true;
                }
                else if (oneChar == lowerCase[i%26])
                {
                    encryptedString += lowerCase[(i+offset)%26];
                    moveOn = true;
                }
                else
                {
                    i++;
                }
            }
        }
        return encryptedString;
    }
}


public class Main 
{
    public static void main (String[] args)
    {
        String inText = "";
        String encryptedText = "";
        int offset = 0;
        String choice;

        Scanner input = new Scanner(System.in);

        System.out.println("Enter something to encrypt or decrypt");
        inText = input.nextLine();

        System.out.println("Would you like to encrypt your message, or decrypt your message? Please type in encrypt or decrypt.");
        choice = input.next();

        System.out.println("How many letters over should the program shift everything? Enter numbers only.");
        offset = input.nextInt();

        Encryptor encrypt = new Encryptor();
        if (choice.equalsIgnoreCase("encrypt") || choice.equalsIgnoreCase("e"))
        {
            encryptedText = encrypt.encryptString(inText, offset);
        }
        else if (choice.equalsIgnoreCase("decrypt") || choice.equalsIgnoreCase("d"))
        {
            offset = 26 - offset;
            encryptedText = encrypt.encryptString(inText, offset);
        }
        System.out.printf("Your message is %s", encryptedText);

        }

    input.close();

    }
}

What do you think of it in terms of overall readability? And for a first attempt, how'd you rank it?

EDIT: Adding in some more questions that occurred overnight.

What things in this make you cringe/confused/ need to be improved? I noticed that during decryption, if the offset goes below 0, I get an index out of range error. For example, if some joker decides to offset more than 26, say 27, the offset becomes -1. This is then subtracted and ran through the remainder and comes out negative. A negative value in an array is an error. I can't simply do the absolute value of the number either as the way my code and arrays are set out, -1%26 would not be 1%26, rather -1%26 should be equivalent to 25.

\$\endgroup\$
6
  • \$\begingroup\$ Good work on your first question! \$\endgroup\$ – Quill Mar 18 '16 at 4:16
  • \$\begingroup\$ Thanks! I figured if I'm to post, may as well be thorough about what I'm seeking. \$\endgroup\$ – ephreal Mar 18 '16 at 4:18
  • \$\begingroup\$ That's cool, but we try to do that in the post. If you have any specific concerns about what you've written (e.g. have I used x variable correctly, this line feels like it could be improved) feel free to add them into the post :) \$\endgroup\$ – Quill Mar 18 '16 at 4:23
  • \$\begingroup\$ Unfortunately, I have no idea what to watch out for yet, hence old title. I'll add to it in the morning if anything more specific comes to mind. \$\endgroup\$ – ephreal Mar 18 '16 at 4:38
  • 1
    \$\begingroup\$ Can you explain what the inner while loop is computing? \$\endgroup\$ – coderodde Mar 18 '16 at 7:22
1
\$\begingroup\$

Besides the implementation of the method I would like to focus on the class design.

encryptString could be a static method because all the instance variables you use are initialized within the method or be final. counter and oneChar could be created inside the loop, the characters array (that should be much greater than that as already stated) could be declared static and final. Moreover I think that Encryptor should have a private constructor so that cannot be instantiated. I would rewrite the class like this.

public class CaesarEncryptor{

    //This constructor is intentionally private to avoid the construction of an object of this class. This class has only static methods.
    private CaesarEncryptor(){}

    public static String encrypt(String input, int offset){
         //...
    }

}

Creating a "static" class would be a little useless for a Object Oriented auto training. To accomplish a more OO oriented encryptor you could: create a generic interface Encryptor with encrypt and decrypt that take just the string and return a string.

A Caesar implementation of Encryptor that in its own constructor take the offset as parameter. Doing that you can add other implementation of Encryptor. Something like:

public interface Encryptor{
    public String encrypt(String toBeEncrypted);
    public String decrypt(String toBeDecrypted);
}

public class CaesarEncryptor implements Encryptor{
    int offset;

    public CaesarEncryptor(int offset){...}

    public String encrypt(String toBeEncrypted){...};
    public String decrypt(String toBeDecrypted){...};
}

Of course the two methods use the same private method that has a parameter to use the offset or its inverse.

The constructor(s) of CaesarEncryptor could take also the input/output characters (or the charset) to avoid special characters not mapped (and throw an exception if one of them is found during encrypt/decrypt.

\$\endgroup\$
2
  • \$\begingroup\$ I'm not sure I follow completely on the static design part nor the generic interface. If I'm to guess, I'll have to find some documents on both and read up. However, when you say the encryptor should have a private constructor, wouldn't that keep me from being able to create an object from the class and use any methods? \$\endgroup\$ – ephreal Mar 18 '16 at 20:40
  • \$\begingroup\$ @ephreal I've edited my answer with some example. \$\endgroup\$ – Fabio F. Mar 18 '16 at 22:46
2
\$\begingroup\$

It would seem that you are restricting yourself to English alphabet, when there is no need for that. All that Caesar cipher does, is adding some constant (usually called key) to each character. Decoding then may be done by subtracting that very key from each encoded character. All in all, I had this in mind:

public class Encryptor {

    public static String encryptString(String input, int key) {
        StringBuilder sb = new StringBuilder(input.length());

        for (char c : input.toCharArray()) {
            sb.append((char)(c + key));
        }

        return sb.toString();
    }

    public static String decryptString(String input, int key) {
        return encryptString(input, -key);
    }
}

Also, using StringBuilder will improve the performance of your cipher: when you do

char c = '?';
myString += c;

a new String is created which runs in linear time as it needs to copy the character content. However, appending to a StringBuilder of a known size runs in constant time.

I run your program, and it does not cipher the white-space. This makes breaking your cipher easier as the attacker will see clearly the lengths of words. Instead, cipher any character in the input string.

\$\endgroup\$
4
  • \$\begingroup\$ I think I'm going to go look up the string builder and figure out it's use. I hadn't even considered the white space, let alone punctuation. I noticed looking through the code now that any punctuation would have been left as it was as well. \$\endgroup\$ – ephreal Mar 18 '16 at 14:17
  • \$\begingroup\$ I set up a second cipher following your model. Does yours add the integer to the ASCII integer, and use that ASCII number as the character it appends to the string? \$\endgroup\$ – ephreal Mar 18 '16 at 21:13
  • \$\begingroup\$ @ephreal Well, as you can see, I add the key to each character whenever encrypting. When, however, decrypting, I simply subtract the key from each encoded character. Since characters are represented by integers, adding a value and then subtracting it brings you back to the original character. \$\endgroup\$ – coderodde Mar 20 '16 at 6:07
  • \$\begingroup\$ Got it. That also explains why when going with large numbers (ex: 50) that I get a ton of question marks showing up. It's trying to represent something that it has no character for. \$\endgroup\$ – ephreal Mar 20 '16 at 21:38

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.