patterncsharpMinor
Shifting chars (Caesar Cipher)
Viewed 0 times
caesarcharsshiftingcipher
Problem
Currently, I'm writing a program that encrypts/decrypts text using Caesars cipher. The code below shifts a character.
This happens by shifting the char around the upper case alphabet then if it was originally a lower case char move it up to the lowercase part of ASCII.
I don't think my current method of doing this is particularly efficient and I can't think of anything to improve it.
/// char to shift
/// amount to shift by
/// letter A in ASCII
/// letter a in ASCII
/// range of alphabet
/// shifted letter
private static char Shifter(char originalChar, int shift, int letterA, int lettera, int letterCount)
=> (char) (letterA + (char.ToUpper(originalChar) - letterA + shift + letterCount) % letterCount
+ (char.IsLower(originalChar) ? lettera - letterA : 0));This happens by shifting the char around the upper case alphabet then if it was originally a lower case char move it up to the lowercase part of ASCII.
I don't think my current method of doing this is particularly efficient and I can't think of anything to improve it.
Solution
private static char Shifter(char originalChar, int shift, int letterA, int lettera, int letterCount)seeing this method declaration makes me shiver.
Let us start with the name of that method.
Shifter is a noun but method names should be made out of a verb or a verb phrase hence Shift would be the way to go. It is always the best if a method declaration can be understood without reading the documentation, so if we rename
int shift to int shiftAmount it will become a lot clearer. The small difference between
int letterA and int lettera can be easily overlooked which is bad. So it better should be for instance int upperA and int lowerA but maybe we should think about this some more. By reading the documentation
/// letter A in ASCII
/// letter a in ASCII I assume this values are always the same
A and a (correct me if I am wrong) hence it doesn't serve any real purpose why this parameters should exist in the method declaration. These values should be extracted to const int either method scoped or private static int class members. /// range of alphabet What is a
range of alphabet ? Should it be the number of characters in an alphabet ? Having a parameter named
originalChar doesn't add any value. Maybe just char letter would be better and easier to understand ?Although you are passing a
char to this method I would change the documentation from /// char to shift to (intergated the name change too)
/// letter to shift I agree with @EthanBierlein about the expression-bodied members part. You really shouldn't do something just because it can be done. The expression-bodied members does have a value but IMO only for small amount of code. Having a line of code which is spawned over two lines is just to much code.
This ternary expression
char.IsLower(originalChar) ? lettera - letterA : 0should better be expressed by a simple
if statement because the result is added to a variable and adding 0 just doesn't make a difference. IMO the indent become more clear. Implementing the mentioned points will then lead to
private static int upperA = 'A';
private static int lowerA = 'a';
/// letter to shift
/// amount to shift by
/// count of letters in alphabet
/// shifted letter
private static char Shift(char letter, int shiftAmount, int alphabetLetterCount)
{
var result = upperA;
result += (char.ToUpper(letter) - upperA + shiftAmount + alphabetLetterCount) % alphabetLetterCount;
if (char.IsLower(letter))
{
result += lowerA - upperA;
}
return (char)result;
}This implementation assumes that the code doesn't need to pass the turkey test.
Code Snippets
private static char Shifter(char originalChar, int shift, int letterA, int lettera, int letterCount)/// <param name="letterA"> letter A in ASCII </param>
/// <param name="lettera"> letter a in ASCII </param>/// <param name="letterCount"> range of alphabet </param>/// <param name="originalChar"> char to shift </param>/// <param name="letter"> letter to shift </param>Context
StackExchange Code Review Q#109793, answer score: 5
Revisions (0)
No revisions yet.