12
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Magus Jan 28 '14 at 17:21
10
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I believe you could simplify the mod operation by using return (Dividend + Divisor) % Divisor; \$\endgroup\$ – Simon Forsberg Jan 28 '14 at 18:09
  • \$\begingroup\$ Good point. I'll change that now. \$\endgroup\$ – Nacimota Jan 28 '14 at 18:12
  • 2
    \$\begingroup\$ 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 \$\endgroup\$ – Bruno Costa Jan 28 '14 at 18:27
  • \$\begingroup\$ @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 \$\endgroup\$ – Nacimota Jan 28 '14 at 18:28
  • 2
    \$\begingroup\$ 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 \$\endgroup\$ – ChrisWue Jan 28 '14 at 19:43
2
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.