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?

• Magus: 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.

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

• Simon Forsberg: I believe you could simplify the mod operation by using return (Dividend + Divisor) % Divisor;
• Nacimota: Good point. I'll change that now.
• Bruno Costa: 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
• Nacimota: @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
• ChrisWue: 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

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