snippetcsharpModerate
How can I improve this Caesar encryption?
Viewed 0 times
thisencryptioncancaesarimprovehow
Problem
I have the following method which encrypts a english text, given the plain text and the desired key:
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?
///
/// 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
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 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.