Caesar Cipher implementation in Java

Question

I wanted to create a class that encrypts provided clear text using Caesar Cipher. Do you find my implementation clean? Any suggestions?

package biz.tugay.caesarcipher;
import java.util.Locale;
/*
 Encyrpts a clear text using Caeser Cipher (https://en.wikipedia.org/wiki/Caesar_cipher)
 with given shift amount.
 Provided shift amount (i.e. key) must be a positive integer less than 26.
 Only English alphabet is supported and encyrpted text will be in uppercase.
 Shift amount 0 will return the same clear text.
 */
public final class CaesarCipher {
 private final String clearText;
 private final int key;
 public CaesarCipher(final String clearText, final int key) {
 if (clearText == null) {
 throw new UnsupportedOperationException("Clear text to be encrypted can not be null!");
 }
 if (key < 0 || key > 26) {
 throw new UnsupportedOperationException("Key must be between 0 and 26");
 }
 this.clearText = clearText;
 this.key = key;
 }
 public String encryptText() {
 final StringBuilder cipherTextBuilder = new StringBuilder();
 final String clearTextUpperCase = clearText.toUpperCase(Locale.US);
 final char[] clearTextUpperCaseCharArray = clearTextUpperCase.toCharArray();
 for (final char c : clearTextUpperCaseCharArray) {
 if (c < 65 || c > 90) { // If the character is not between A .. Z, append white space.
 cipherTextBuilder.append(" ");
 continue;
 }
 final Character encryptedCharacter = encryptCharacter(c);
 cipherTextBuilder.append(encryptedCharacter);
 }
 return cipherTextBuilder.toString();
 }
 private Character encryptCharacter(final char c) {
 final int initialShift = c + key;
 final int finalShift;
 if (initialShift > 90) {
 // This is the case where we go beyond Z, we must cycle back to A.
 finalShift = (initialShift % 90) + 64;
 } else {
 // We are in the boundries so no need to cycle..
 finalShift = initialShift;
 }
 return (char) finalShift;
 }
}

and I have 2 simple tests:

package biz.tugay.caesarcipher;
import org.junit.Assert;
import org.junit.Test;
public class CaesarCipherTest {
 @Test
 public void shouldReturnBCDForClearTextABCAndKey1() {
 final String clearText = "abc";
 final CaesarCipher caesarCipher = new CaesarCipher(clearText, 1);
 final String encryptedText = caesarCipher.encryptText();
 Assert.assertTrue(encryptedText.equals("BCD"));
 }
 @Test
 public void shouldReturnAForZAndKey1() {
 final String clearText = "Z";
 final CaesarCipher caesarCipher = new CaesarCipher(clearText, 1);
 final String encryptedText = caesarCipher.encryptText();
 Assert.assertTrue(encryptedText.equals("A"));
 }
}

• asgs: 1. Replace UnsupportedOperationException with IllegalArgumentException at least for the clearText validation. 2. Change the error message to "Key must be from 0 to 26". 3. To cycle the character, I find subtracting 90 from it to be intuitive than doing a remainder operation.

Answer 1

package biz.tugay.caesarcipher;
import java.util.Locale;
/*
 Encyrpts a clear text using Caeser Cipher (https://en.wikipedia.org/wiki/Caesar_cipher)
 with given shift amount.

There is a typo in "Encrypts", and the URL should be a hyperlink, like <a href="https://...">Caesar Cipher</a>.

 Provided shift amount (i.e. key) must be a positive integer less than 26.

Depending on who you ask, the word positive may exclude the 0. You should just say must be between 0 and 25.

But even more important: the documentation must match the code. Currently the code says <= 26.

 Only English alphabet is supported and encyrpted text will be in uppercase.

The same typo as above. And, the Turkish lowercase dotless i (ı) is also "supported", although not intentionally.

 Shift amount 0 will return the same clear text.
 */
public final class CaesarCipher {
 private final String clearText;

There is no reason that the Cipher class ever stores the clear text. Therefore this field should be replaced with a method parameter.

 private final int key;
 public CaesarCipher(final String clearText, final int key) {
 if (clearText == null) {
 throw new UnsupportedOperationException("Clear text to be encrypted can not be null!");
 }
 if (key < 0 || key > 26) {
 throw new UnsupportedOperationException("Key must be between 0 and 26");
 }
 this.clearText = clearText;
 this.key = key;
 }
 public String encryptText() {
 final StringBuilder cipherTextBuilder = new StringBuilder();
 final String clearTextUpperCase = clearText.toUpperCase(Locale.US);
 final char[] clearTextUpperCaseCharArray = clearTextUpperCase.toCharArray();
 for (final char c : clearTextUpperCaseCharArray) {
 if (c < 65 || c > 90) { // If the character is not between A .. Z, append white space.

This comment is redundant if you write c < 'A' || c > 'Z'.

 cipherTextBuilder.append(" ");
 continue;
 }
 final Character encryptedCharacter = encryptCharacter(c);
 cipherTextBuilder.append(encryptedCharacter);
 }
 return cipherTextBuilder.toString();
 }
 private Character encryptCharacter(final char c) {
 final int initialShift = c + key;
 final int finalShift;
 if (initialShift > 90) {
 // This is the case where we go beyond Z, we must cycle back to A.
 finalShift = (initialShift % 90) + 64;
 } else {

This can never happen since you already check the condition in the encryptText method.

 // We are in the boundries so no need to cycle..
 finalShift = initialShift;
 }
 return (char) finalShift;
 }
}

I would write the encryptCharacter method as follows and remove the bounds check from the encryptText method:

private char encryptCharacter(char c) {
 if ('A' <= c && c <= 'Z') {
 int position = c - 'A';
 int shiftedPosition = (position + shift) % 26;
 return (char) ('A' + shiftedPosition);
 } else {
 return c; // Or ' ', as in your code
 }
}

The tests

package biz.tugay.caesarcipher;
import org.junit.Assert;
import org.junit.Test;
public class CaesarCipherTest {
 @Test
 public void shouldReturnBCDForClearTextABCAndKey1() {
 final String clearText = "abc";
 final CaesarCipher caesarCipher = new CaesarCipher(clearText, 1);
 final String encryptedText = caesarCipher.encryptText();
 Assert.assertTrue(encryptedText.equals("BCD"));
 }

I usually split each test into three paragraphs. The first prepares everything, the second does the interesting work, and the third asserts that the result is correct. When you follow this style, you can easily see which part of the code is worth stepping through with a debugger.

Instead of assertTrue, you should call assertEquals("BCD", encryptedText). Because when that assertion fails, the error message is much nicer, giving you the expected and the actual result.

 @Test
 public void shouldReturnAForZAndKey1() {
 final String clearText = "Z";
 final CaesarCipher caesarCipher = new CaesarCipher(clearText, 1);
 final String encryptedText = caesarCipher.encryptText();
 Assert.assertTrue(encryptedText.equals("A"));
 }
}

You forgot to test lowercase letters and non-alphabetic characters. And emojis, which by the way would result in two spaces per emoji.

Answer 2

Here are some additional thoughts in addition to what @RolandIllig wrote:

Magic Numbers

Roland already suggested to use the cacharter representation isstead if the ASCII codes of the chars. But then they are still magic number. You better create constnts for them with names that convey their meaning for the program:

private final char LOWEST_CHAR_ALLOWED = 'a';
private final char HIGHEST_CHAR_ALLOWED = 'z';

Avoid continue

for (final char c : clearTextUpperCaseCharArray) {
 if (c < 65 || c > 90) { // If the character is not between A .. Z, append white space.
 cipherTextBuilder.append(" ");
 continue;
 }
 final Character encryptedCharacter = encryptCharacter(c);
 cipherTextBuilder.append(encryptedCharacter);
 }

The use of continue hides that the 2 lines after the if are the else block. You better write it this way:

for (final char c : clearTextUpperCaseCharArray) {
 if (c < 65 || c > 90) { // If the character is not between A .. Z, append white space.
 cipherTextBuilder.append(" ");
 } else {
 final Character encryptedCharacter = encryptCharacter(c);
 cipherTextBuilder.append(encryptedCharacter);
 }

Single Responsibility Pattern

This same if does two things: it replaces the char with space or encrypts it and adds the converted charr to the result.

You could separate it that way:

for (final char c : clearTextUpperCaseCharArray) {
 Character encryptedCharacter
 if (c < LOWEST_CHAR_ALLOWED || c > HIGHEST_CHAR_ALLOWED) { 
 encryptedCharacter = SINGE_PSACE;
 } else {
 encryptedCharacter = encryptCharacter(c);
 }
 cipherTextBuilder.append(encryptedCharacter);

Singe Abstaction Layer

Methods should either call other methods or do some "primitive" calculations. Your method encryptText() does both.

the if should be moved to the encryptCharacter() method:

for (final char c : clearTextUpperCaseCharArray) {
 cipherTextBuilder.append(encryptCharacter(c));
// ...
private Character encryptCharacter(final char c) {
 if (c < LOWEST_CHAR_ALLOWED || c > HIGHEST_CHAR_ALLOWED) { 
 return SINGE_PSACE;
 } else { // not really needed but enhances readability.
 final int initialShift = c + key;
 final int finalShift;

• Roland Illig: In a rot13 encryption program, the character literals for 'A' and 'Z' are not magic by any means. Therefore there is no reason to introduce constants for them. Also, the names of the constants are wrong: in rot13, all characters are allowed, it's just that they stay the same during encryption.
• Timothy Truckle: @RolandIllig "In a rot13 encryption program, the character literals for 'A' and 'Z' are not magic by any means." The fact the the OP chose them lower case and you chose them upper case show: they are.
• Koray Tugay: I really like 'continue', I only disagree with you on that one.
• Roland Illig: The OP did choose uppercase, and I followed that decision. Lowercase would have been 97 to 122.
• Timothy Truckle: continue is the poor mans goto It almost ever hids alternative paths as it does in your case and also usually stands i the way when you want to apply the extract method refactoring.