HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Caesar Cipher implementation in Java

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationcaesarjavacipher

Problem

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 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 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 = (initialSh

Solution

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 Caesar Cipher.

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 '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.

Code Snippets

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.

Context

StackExchange Code Review Q#158145, answer score: 4

Revisions (0)

No revisions yet.