How to simplify my implementation of a basic Caesar Shift Encryption algorithm c#?

So I have recently decided that my coding style is somewhat clunky. The trouble is I never seem to be able to get to a stage where I can figure out the way of simplifying it into less lines of more efficient code.

I was in a team coding situation the other day trying to code a word wrapping feature using TDD. When I was in the driving seat I spent most of my time with String.Split() and if statements this and if that. The guy I was coding with asked why so complex and went with the simpler recursive one liner that returned the values required and with a few conditions to boot him out of the recursive loop when he was done.

So my question is thus – Below is some code that I h开发者_JAVA百科ave written to do a Caesar Shift Encryption on a string input using only lower case alphabet and no spaces. The unit tests pass and I believe I have implemented the various conditions that may occur.

In a nut shell how would you guys simplify the code below to make it more readable and more efficient?

I appreciate the help on this, because at the end of the day I need to make my coding style less verbose and more simplistic and can’t figure out the best place to start.

Cheers

Code in C#:

 public static string Encrypt(string inputString, int shiftPattern)
    {
        StringBuilder sb = new StringBuilder();

        char[] alphabet = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z' };

        //y = x + 3 (mod 26)
        foreach (var letter in inputString.ToLower())
        {
            if (!alphabet.Contains(letter))
            {
                return "The " + letter + " Character was not in the sample set, please ensure you only use letters";
            }

            var res = Array.IndexOf(alphabet, letter) + (shiftPattern % 26);

            if (res >= 26)
            {
                res = res - alphabet.Length;

                sb.Append(alphabet[res]);
            }

            else if (res < 0)
            {
                res = alphabet.Length + res;
                sb.Append(alphabet[res]);
            }

            else

                sb.Append(alphabet[res]);
        }
        return sb.ToString();
    }

1.- Put sb.Append(alphabet[res]) only once outside the conditionals.
2.- Consider throw an exception instead of return a message... so later you can easily check that the operation works propertly. You should also consider let the character 'As Is' if It's not pressent in your alphabet definition. It will allow you deal with whitespaces, etc.
3.- Check that the last conditional are really necessary. At first view... It looks that it could be safety removed... so confirm it. We can add a Math.Abs function to avoid problems with negative numbers in ShiftPattern.
4.- In some places you use alphabet.Length, and in other places you use 26. Use always alphabet.Length.
5.-Don't check that (res >= 26), you can do directly a MOD operation.

    public static string Encrypt(string inputString, int shiftPattern)
            {
                StringBuilder sb = new StringBuilder();

                char[] alphabet = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z' };

                //y = x + 3 (mod 26)
                foreach (var letter in inputString.ToLower())
                {
                    if (!alphabet.Contains(letter)) //Consider throwing and exception instead
                    {
                        return "The " + letter + " Character was not in the sample set, please ensure you only use letters";
                    }

                    var res = Array.IndexOf(alphabet, letter) + (Math.Abs(shiftPattern) % alphabet.Length);
                    res = res % alphabet.Length
                    sb.Append(alphabet[res]);
                }
                return sb.ToString();
            }

You can drop the array and everything in the for loop in one line of code.

        StringBuilder sb = new StringBuilder();
        foreach (var letter in inputString.ToLower())
            sb.Append((char)((((letter - 'a') + shiftpattern)%26) + 'a'));

In: zombie & shift by 1

Out: apncjf

There is no need to hold the complete alphabet list.
Also, if you don't support spaces then your messages will be like roman latin, without spaces...

    public static string Encrypt(string inputString, int shiftPattern)
    {
        StringBuilder sb = new StringBuilder();
        foreach(char letter in inputString.ToLower())
        {
            int encryptedValue = 0;
            if (letter == ' ')
            {
                encryptedValue = ' ';
            }
            else
            {
                encryptedValue = (((letter - 'a') + shiftPattern) % 26) + 'a';
            }

            sb.Append((char)encryptedValue);
        }
        return sb.ToString();
    }

I would recommend the following,

1. in case or more than two conditions use Switch- Case.
2. Instead of char[] or any variable declaration use var if using 3.0 or above
3. For and Foreach loops are avoided using lambda expressions.
4. Make such methods static and put it in some common class that can be shared globally.

For more good practice . Follow Resharper tool from jetbrains

With some changes to the algorithm :

static char[] alphabet = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z' };

public static string Encrypt(string inputString, int shiftPattern)
{
    if (inputString == null) return null;
    if (shiftPattern <= 0)
    {
        shiftPattern = alphabet.Length + shiftPattern % alphabet.Length;
    }

    var chars = inputString.Select(c =>
    {
        var index = Array.IndexOf(alphabet, char.ToLower(c));
        if (index == -1) return c;

        var result = alphabet[(index + shiftPattern) % 26];
        return char.IsLower(c) ? result : char.ToUpper(result);
    });

    return new string(chars.ToArray());
}

• Manage the null string case.
• Non-cypherable chars are kept as-is (debatable in a cypher...)
• Just use linq instead of old style loops and builders.
• Manage upper case
• Shorter, while still understandable without problems.
• I separated the return from the select for clarity here, in normal code if the return ... Select line didn't go over the column limit for the code base I would have combined the two.