Cipher decryption and encryption

Question

I'm brand new to C# and I've started to make a cipher decryption program. The code works and runs fine, apart from the odd exception. I'm just looking for ways to improve my code and make more efficient.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.IO;
namespace Application
{
class Caesar
{
 static void Main(string[] args)
 {
 try{
 Console.WriteLine("Enter Key:");
 int k = Convert.ToInt32(Console.ReadLine());
 Console.WriteLine("Press E for Encrypting and D for Decrypting:");
 string choice =Convert.ToString(Console.ReadLine()).ToUpper();
 switch (choice)
 {
 case "E": Console.Write("Enter Plain Text:");
 string pt = Console.ReadLine();
 caesar_cipher(k, pt);
 break;
 case "D": Console.Write("Enter Cipher :");
 string ct = Console.ReadLine();
 caesar_decipher(k, ct);
 break;
 default: Console.WriteLine("You've entered an incorrect option!");
 break;
 }
 }
 catch(Exception)
 {
 Console.WriteLine ("The value you entered is incorrect");
 Console.WriteLine ("Press any key to try again");
 }
 }
 static void caesar_cipher(int key, string pt)
 {
 int size = pt.Length;
 char[] value = new char[size];
 char[] cipher = new char[size];
 for (int r = 0; r < size; r++)
 {
 value[r] = Convert.ToChar(pt.Substring(r, 1));
 }
 for (int re = 0; re < size; re++)
 {
 int count = 0;
 int a = Convert.ToInt32(value[re]);
 for (int y = 1; y <= key; y++)
 {
 if (count == 0)
 {
 if (a == 90)
 { a = 64; }
 else if (a == 122)
 { a = 96; }
 cipher[re] = Convert.ToChar(a + y);
 count++;
 }
 else
 {
 int b = Convert.ToInt32(cipher[re]);
 if (b == 90)
 { b = 64; }
 else if (b == 122)
 { b = 96; }
 cipher[re] = Convert.ToChar(b + 1);
 }
 }
 }
 string ciphertext = "";
 for (int p = 0; p < size; p++)
 {
 ciphertext = ciphertext + cipher[p].ToString();
 }
 Console.WriteLine("Cipher Text=");
 Console.WriteLine(ciphertext.ToUpper());
 }
 static void caesar_decipher(int key, string ct)
 {
 int size = ct.Length;
 char[] value = new char[size];
 char[] cipher = new char[size];
 for (int r = 0; r < size; r++)
 {
 cipher[r] = Convert.ToChar(ct.Substring(r, 1));
 }
 for (int re = 0; re < size; re++)
 {
 int count = 0;
 int a = Convert.ToInt32(cipher[re]);
 for (int y = 1; y <= key; y++)
 {
 if (count == 0)
 {
 if (a == 65)
 { a = 91; }
 else if (a == 97)
 { a = 123; }
 value[re] = Convert.ToChar(a - y);
 count++;
 }
 else
 {
 int b = Convert.ToInt32(value[re]);
 if (b == 65)
 { b = 91; }
 else if (b == 97)
 { b = 123; }
 value[re] = Convert.ToChar(b - 1);
 }
 }
 }
 string plaintext = "";
 for (int p = 0; p < size; p++)
 {
 plaintext = plaintext + value[p].ToString();
 }
 Console.WriteLine("Plain Text=");
 Console.WriteLine(plaintext.ToLower());
 }
}
}

• anon: You need to figure out how you want to indent things. There are several styles, all perfectly valid options, but they all share one trait: Consistency. Indents, for example, tend to always be the same size for all control statements – it's not one space after try and four after anything else.

Answer 1

Code bloat

A simple task is simple. Almost 150 lines of code are not justified for such an easy task. You imported Linq so you arguably know about it, actually using it may do very good to your code.

A general, pseudo-code view of the caesarCipher function may be:

public static string caesarEncrypt(string text, int key) {
 return text.Map( x -> alphabet.include?(x) ? shiftChar(x, key) : x );
}

shiftChar may be written as:

return alphabet [ alphabet.index(char) % alphabet.length ]

And you are done.

And caesarDecrypt is just caesarEncrypt with key = 26 - key, there is no need to write the whole thing again.

• Robert Snyder: the top 4 using statements the OP has are the default for a new file. I'm going to guess that the OP either A) didn't know that those can be removed if they are not being used or B) doesn't know about linq. or C) both

Answer 2

Magic strings + magic numbers = magic code

My suggestions:

Create constants for E and D:

class Shortcuts
{
 const string Encrypt = "E";
 const string Decrypt = "D";
}

then your switch will be easier to understand:

switch (choice)
{
 case Shortcuts.Encrypt:
 // ...
}

---

65, 97, 123 ?

Give them a meaning. Like "a", or "z", or "A"

Here the same:

for (int r = 0; r < size; r++)
for (int re = 0; re < size; re++)

You should use whole words if these letters have a meaning or just i as an index which is clear to anyone. r or re are no longer easy to understand.

---

By promoting your magic number to constants you can convert some of your ifs into switches:

if (b == 65)
{ b = 91; }
else if (b == 97)
{ b = 123; }
value[re] = Convert.ToChar(b - 1);

switch(b)
{
 case AsciiCodes.A:
 b = AsciiCodes.Something; 
 break;
}

But a dictionary would be even better:

var cipherDic = new Dictionary<int, int>
{
 { AsciiCodes.A, AsciiCodes.Something }
};
b = cipherDic[b];

• t3chb0t: mhmm, why downvote?

Answer 3

Avoid wrapping all the code of the method in a try..catch block. Here are some ways to deal with exceptions.

Also, be consistent with spacing and indentation in order to improve readability.