6
\$\begingroup\$

I use these in a windows form with fun buttons, I wanted to see what the world thinks of my coding.

This is something that I coded about 2-3 years ago, I did change some of the coding when I looked at it and thought "this could be better written like this"

Let me know what you think and what I could do differently.

Public class shiftCipher
{
    public string removeSpaces(string plainIn)
    {
        plainIn = plainIn.Replace(" ", string.Empty);
        return plainIn;
    }

    public string reverse(string plainIn)
    {
        char[] charArray = plainIn.ToCharArray();
        Array.Reverse(charArray);

        return new string(charArray);
    }

    public string toUCase(string strIn)
    {
        strIn = strIn.ToUpper();
        return strIn;
    }

    public string shift(string plainIn, int shift)
    {// 65=a 90=z in ASCII
        string readOut=string.Empty;
        char[] charArray;

        charArray = plainIn.ToCharArray();
        for (int i = 0; i < plainIn.Length; i++)
        {
            int Num = Convert.ToInt32(charArray[i]) + shift;
            readOut += Convert.ToChar(Num > 90 ? Num -= 26 : (Num < 65 ? Num += 26: Num));
        }
        return readOut;
    }

    //this method will take all of these methods and put them together to encrypt
    public string Encrypt(string ReadIn, int Shift)
    {
        ReadIn = removeSpaces(ReadIn);
        ReadIn = reverse(ReadIn);
        ReadIn = toUCase(ReadIn);
        ReadIn = shift(ReadIn, Shift);
        return ReadIn;
    }

    public string Decrypt(string ReadIn, int amountShift)
    {
        ReadIn = reverse(ReadIn);// undo the reverse by running the method again
        ReadIn = shift(ReadIn, -amountShift);

        return ReadIn;
    }       
}

In the Decrypt I didn't think there was a good way to put the spaces back into the string, so I didn't try to take that on, and I also left the string in uppercase if it was there.

\$\endgroup\$

3 Answers 3

8
\$\begingroup\$

Simpler Methods.

the code:

public string removeSpaces(string plainIn)
{
    plainIn = plainIn.Replace(" ", string.Empty);
    return plainIn;
}

Could be:

public string removeSpaces(string plainIn)
{
    return plainIn.Replace(" ", string.Empty);
}

Which begs the question... why have the removeSpaces method at all?

The same is true with toUCase()

Simpler Conversion

Why use Convert.ToInt32(....); and Convert.ToChar(...) when (int) and (char) will do?

Simpler Math

This line here (the crux of the problem), can be simplified using modulo and removing the Magic Numbers:

int Num = Convert.ToInt32(charArray[i]) + shift;
readOut += Convert.ToChar(Num > 90 ? Num -= 26 : (Num < 65 ? Num += 26: Num));

can be:

// Constants....
private const int LETTERA = (int)'A';
private const int LETTERCOUNT = (int)'Z' - LETTERA + 1;

// actual code.
 readOut += (char)(LETTERA + (((int)charArray[i] + shift - LETTERA + LETTERCOUNT) % LETTERCOUNT));

(See ideone here)

Without the conditional / ternary operator it is more likely to be tighter/faster code as well.

Comments

You have used magic numbers, so you attempted to document them with a comment:

public string shift(string plainIn, int shift)
{// 65=a 90=z in ASCII

But, the comments are wrong, and misleading (wrong case character).

 {// 65=A 90=Z in ASCII

Symmetry

When you have methods that do symmetrical work (they are closely related), then you should ensure the parameter names are the same.... For example, you have:

public string Encrypt(string ReadIn, int Shift)

and

public string Decrypt(string ReadIn, int amountShift)

The second should have the parameter name Shift

\$\endgroup\$
2
  • \$\begingroup\$ I perhaps should have suggested that modification to your answer :p. This review is on spot. \$\endgroup\$ May 31, 2014 at 23:06
  • 2
    \$\begingroup\$ The LETTERA constant seems dangerously close to const int ONE = 1;, etc.. Why not just use 'A' directly? Otherwise, FIRST_LETTER and maybe add a LAST_LETTER. \$\endgroup\$
    – Bob
    Jun 1, 2014 at 7:53
7
\$\begingroup\$

There is certainly no need for the alias method ToUpper of string so get rid of it. Also implement your string methods in a string utility class and use c# naming conventions.

@rolfl was very convenient about your comment but you can do it even better! do a cast of the character to int, so you don't have to consult ascii table.

Edit You should also avoid string concatenation, create a new Array instead.

public static class StringUtils{
    public static string Reverse(this string value){
        char[] charArray = plainIn.ToCharArray();
        Array.Reverse(charArray);
        return new string(charArray);
    }

    public static string RemoveSpaces(this string value){
        return plainIn.Replace(" ", string.Empty);
    }
}


public class ShiftCipher
{

    public string Shift(string plainIn, int shift)
    { 
        int z = (int)'Z';
        int a = (int)'A';
        char[] output = new char[plainIn.Length];

        for (int i = 0; i < plainIn.Length; i++)
        {
            int num = plainIn[i] + shift;
            output[i] = Convert.ToChar(num > z ? num -= (z-a) : (num < z ? num += (z-a): num));
        }
        return new string(output);
    }

    public string Encrypt(string message, int shiftN)
    {
        message = message.RemoveSpaces().ToUpper().Reverse();
        return Shift(message, shiftN);
    }

    public string Decrypt(string message, int amountShift)
    {
        return Shift(message.Reverse() , -amountShift);
    }       
}
\$\endgroup\$
3
  • \$\begingroup\$ I like the String Extensions. that was kind of fun to figure out, and I like it. I was wanting to be able to use those on the other Ciphers that I was going to make, but thought I was going to have to copy paste then methods. this is better \$\endgroup\$
    – Malachi
    May 31, 2014 at 23:01
  • \$\begingroup\$ Yup extension methods certainly are useful in many scenarios. The ability to call a static method as if it was an instance method is awesome. \$\endgroup\$ May 31, 2014 at 23:11
  • \$\begingroup\$ @Malachi The convertion of a char to int is also made implictly but i usually like to include the explicit cast. \$\endgroup\$ May 31, 2014 at 23:23
2
\$\begingroup\$

If you treat the standard ASCII printable character set(32-255) the same you don't have to worry about upper,lower, space etc., only filter out character codes above and below that, and each encrypted string will decrypt to the exact same string. This also greatly simplifies the algorithm since the character plus/minus the shift minus 32 modulo 224 plus 32 is the new character:

public static string EncryptDecrypt(string input, int shift, bool encrypt = true)
{
    if(!encrypt)
        shift *= -1;
    StringBuilder sb = new StringBuilder();
    for(int i = 0; i < input.Length; i++)
    {
        if(input[i] > 31 && input[i] < 256)
        {
            sb.Append((char)((((input[i] + shift) - 32) % 224) + 32));
        }
        else
            sb.Append(input[i]);
    }
    return sb.ToString();
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.