9
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Looks good, although I would go for 'result %= letterCount' for the 4th line and compound the 6th and 7th lines into 'return (char)result' \$\endgroup\$ – RedLaser Nov 4 '15 at 22:11
  • \$\begingroup\$ @RedLaser edited. :) \$\endgroup\$ – Ethan Bierlein Nov 4 '15 at 22:12
  • \$\begingroup\$ I'll hold up on accepting in case anyone else has a better answer though as it hasn't been up that long \$\endgroup\$ – RedLaser Nov 4 '15 at 22:12
  • \$\begingroup\$ @RedLaser That's perfectly fine with me. :) \$\endgroup\$ – Ethan Bierlein Nov 4 '15 at 22:13
5
\$\begingroup\$
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.

\$\endgroup\$
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Looks interesting but I think I'll stay with how I'm doing it for now. Thanks anyway \$\endgroup\$ – RedLaser Nov 4 '15 at 22:15

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.