How can I improve this Caesar encryption?

Question

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

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

    for (int i = 0; i < plainText.Length; i++)
    {
        if (plainText[i].ToString() == " ")
        {
            encText += " ";
        }

        for (int j = 0; j < alphabet.Length; j++)
        {
            if (String.Equals(alphabet[j].ToString(), plainText[i].ToString(), StringComparison.OrdinalIgnoreCase))
            {
                try
                {
                    encText += alphabet[j + keyValue];
                }
                catch (Exception ex)
                {
                    if (j + keyValue > 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?

Answer 1

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

Answer 2

A simple solution is to have 2 arrays, one with chars to map from and one with chars to map to.

Then a simple look up, something like this, is all that is necessary

char[] mapFrom = "ABC".ToCharArray();
char[] mapTo   = "LMN".ToCharArray();

mapTo[Array.IndexOf(mapFrom, ch)] 

Advantages include a fast look-up, simple easy to understand code, and the fact that the 2 array could be either hard-coded or generated using the desired algorithm (only once).