# How can I improve this Caesar encryption?

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?

• Often, when I see a for-if or a foreach-if, I assume there's a missing Where. When I see nested fors, I assume there's a missing SelectMany. Jan 28, 2014 at 17:21

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

• I believe you could simplify the mod operation by using return (Dividend + Divisor) % Divisor; Jan 28, 2014 at 18:09
• Good point. I'll change that now. Jan 28, 2014 at 18:12
• You can also substitute your alphabet.Contains O(n) for c - 'A' < alphabet.Length O(1). You can also use that value as a indexer so you don't have to use IndexOf. pastebin.com/CPDWh19J Jan 28, 2014 at 18:27
• @Simon Actually it just occurred to me that return (Dividend + Divisor) % Divisor; doesn't work quite the same way as the original implementation and can still end up returning a negative number Jan 28, 2014 at 18:28
• You have a ) too much in your Mod call in the second version - Should be Mod(idx + keyValue, 26). Also if you make keyValue an unsigned int then you can get rid of the Mod method entirely because then you know idx + keyValue will never be negative. Also standard naming conventions for method parameters is camelCase Jan 28, 2014 at 19:43

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