HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Shifting chars (Caesar Cipher)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
caesarcharsshiftingcipher

Problem

Currently, I'm writing a program that encrypts/decrypts text using Caesars cipher. The code below shifts a character.

///  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 : 0


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