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

How can I improve this Caesar encryption?

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

Problem

I have the following method which encrypts a english text, given the plain text and the desired key:

/// 
/// Encrypts english plain text using user defined key. Range for the key is from -25 to 25.
/// 
/// 
/// 
/// 
public static string encryptTextEng(String plainText,int keyValue)
{
    string encText = "";
    char[] alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();

    for (int i = 0; i  26)
                    {
                        encText += alphabet[j + keyValue - 26];
                    }
                    else if (j + keyValue < 0)
                    {
                        encText += alphabet[j + keyValue + 26];
                    }
                }
            }
        }
    }

    return encText;
}


What are good programming practices I am missing here? Also I am not sure about the try/catch part I guess there are better ways to do it? Would it be a good idea to add a switch case which checks if the char it is currently reading is a punctuation character?

Solution

You are correct in suspecting that the try/catch is not necessary. Generally speaking, Caesar ciphers are implemented using modular arithmetic. We can determine if a character is alphabetical by simply checking if alphabet contains that character; anything else (whitespace, digits, punctuation, etc.) can just be copied as-is, so neither the second for loop nor the proposed switch are necessary either. Here's an example of how you could simplify your code:

public static string encryptTextEng(string plainText, int keyValue)
{
    string encText = "";
    char[] alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();

    foreach (char c in plainText.ToUpper())
    {
        if (alphabet.Contains(c))
        {
            encText += alphabet[Mod((Array.IndexOf(alphabet, c) + keyValue), 26)];
        }
        else
        {
            encText += c;
        }
    }

    return encText;
}

private static int Mod(int dividend, int divisor)
{
    int remainder = dividend % divisor;
    return remainder < 0 ? remainder + divisor : remainder;
}


I threw the Mod method in there because the modulo operator does not work as you might expect with negative numbers.

As mentioned by Bruno in the comments below, a more performant way of checking if the current character is in alphabet and getting the index of that character would be:

foreach (char c in plainText.ToUpper())
{
    int idx = c - 'A';
    if (idx >= 0 && idx < alphabet.Length)
    {
        encText += alphabet[Mod(idx + keyValue, 26)];
    }
    else
    {
        encText += c;
    }
}

Code Snippets

public static string encryptTextEng(string plainText, int keyValue)
{
    string encText = "";
    char[] alphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();

    foreach (char c in plainText.ToUpper())
    {
        if (alphabet.Contains(c))
        {
            encText += alphabet[Mod((Array.IndexOf(alphabet, c) + keyValue), 26)];
        }
        else
        {
            encText += c;
        }
    }

    return encText;
}

private static int Mod(int dividend, int divisor)
{
    int remainder = dividend % divisor;
    return remainder < 0 ? remainder + divisor : remainder;
}
foreach (char c in plainText.ToUpper())
{
    int idx = c - 'A';
    if (idx >= 0 && idx < alphabet.Length)
    {
        encText += alphabet[Mod(idx + keyValue, 26)];
    }
    else
    {
        encText += c;
    }
}

Context

StackExchange Code Review Q#40277, answer score: 10

Revisions (0)

No revisions yet.