First try Caesar Cipher

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);
}

}

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.

• Good work on your first question! Commented Mar 18, 2016 at 4:16
• Thanks! I figured if I'm to post, may as well be thorough about what I'm seeking. Commented Mar 18, 2016 at 4:18
• 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 :) Commented Mar 18, 2016 at 4:23
• 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. Commented Mar 18, 2016 at 4:38
• Can you explain what the inner while loop is computing? Commented Mar 18, 2016 at 7:22

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.

• 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? Commented Mar 18, 2016 at 20:40
• @ephreal I've edited my answer with some example. Commented Mar 18, 2016 at 22:46

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.

• 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. Commented Mar 18, 2016 at 14:17
• 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? Commented Mar 18, 2016 at 21:13
• @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. Commented Mar 20, 2016 at 6:07
• 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. Commented Mar 20, 2016 at 21:38