Shifting chars (Caesar Cipher)

Question

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

    /// <param name="originalChar"> char to shift </param>
    /// <param name="shift"> amount to shift by </param>
    /// <param name="letterA"> letter A in ASCII </param>
    /// <param name="lettera"> letter a in ASCII </param>
    /// <param name="letterCount"> range of alphabet </param>
    /// <returns> shifted letter </returns>
    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.

Answer 1

Sorry for the small review, I couldn't find much else that was wrong.

First off, I'm not sure that this function needs to be a expression-bodied member, and since you aren't really using them the way you should, you should change it to a normal method:

private static char Shifter(char originalChar, int shift, int letterA, int lettera, int letterCount)
{
    return (char) (letterA + (char.ToUpper(originalChar) - letterA + shift + letterCount) % letterCount 
                + (char.IsLower(originalChar) ? lettera - letterA : 0));
}

I'd also split out the return statement into separate lines, like this as well:

private static char Shifter(char originalChar, int shift, int letterA, int lettera, int letterCount)
{
    var result = letterA; 
    result += (char.ToUpper(originalChar) - letterA + shift + letterCount) % letterCount; 
    result += char.IsLower(originalChar) ? lettera - letterA : 0;
    return (char)result;
}

Answer 2

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

/// <param name="letterA"> letter A in ASCII </param>
/// <param name="lettera"> letter a in ASCII </param>  

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.

/// <param name="letterCount"> range of alphabet </param>  

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

/// <param name="originalChar"> char to shift </param>  

to (intergated the name change too)

/// <param name="letter"> letter to shift </param>  

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';
/// <param name="letter"> letter to shift </param>
/// <param name="shiftAmount"> amount to shift by </param>
/// <param name="alphabetLetterCount"> count of letters in alphabet </param>
/// <returns> shifted letter </returns>
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.

Answer 3

Using a look up table is an optimal way to doing character replacement. It can be implemented using arrays, list, dictionaries etc.

You can hard code the lookup data or generate it, using a function similar to yours.

See my answer to a similar question here. https://codereview.stackexchange.com/a/40335/35554