2
\$\begingroup\$

I am learning C# and for an experiment I tried implementing the Caesar Cypher cryptography method which just offsets every character of the string n characters. How can I make this simple program better?

string original = Console.ReadLine();
string encrypted = "";
int key = 1;
foreach (var l in original)
{
    if (!l.Equals(' '))
    {
        if (l.Equals('z'))
        {
            char next = 'a';
            encrypted += next;
        }
        else
        {

            double next = (int)l + key;
            encrypted += (char)next;
        }
    }
    else
    {
        encrypted += ' ';
    }
}
Console.WriteLine(encrypted);
\$\endgroup\$
3
\$\begingroup\$

Afew things I noticed:

Concatenating strings using the plus operator(+), is very inefficient. Each concatenation creates a new string. This due to strings being immutable. Using a char array or the StringBuilder is much more efficient.

The variable next is a double but the value you're assigning to it is 2 ints added together. It would make much more sense to have this as a char. In fact you should declare this before the if block to be able to use it in each condition.

Equals isn't doing what you appear to think it does. Equals is for testing whether 2 objects are the same. == is for testing whether 2 objects values are the same.

You appear to expect only lower case characters and spaces. However you don't seem to have any mechanism for testing whether the string complies with that.

You can simplify getting the replacement character by using the modulus(%) operator. Something like this should work: newChar = (char)(((oldChar - 'a') + key) % 26) + 'a'

Your code won't work right if the string has words from a language that uses two bytes to represent one character.

Right now you've hardcoded the key. It would be better to have a method that takes the string and the key as parameters.

\$\endgroup\$
1
\$\begingroup\$

Think Objects

This problem has 2, readily apparent, main parts, the message to be encrypted and the "cipher machine" that encrypts it. Thinking through the problem identifying its parts is the way to start out on the Yellow Brick Road of OO goodness. So we already have enough start writing code!

public string message;

public class CaesarCipher {
    public string encrypt (string clearText) { ... }
}

Now it's suddenly obvious we need to decrypt too!

public class CaesarCipher {
    public string encrypt (string clearText) { ... }
    public string decrypt (string secretText) { ... }
} 

Your code is suggestive of the IEnumerator interface, which has a MoveNext() method and Current (position) property. This allows foreach statements on a CaesarCipher object:

public class CaesarCipher : IEnumerator { ... }

CaesarCipher littleOrphanAnnieDecoderRing = new CaesarCipher();

foreach ( var alphaBit in littleOrphanAnnieDecoderRing ) { ... }

FORGET THIS FOR NOW. It's advanced and confusing. The Wikipedia article shows a simple modulus arithmetic formula


Learn to count from zero and be a hero

If you can read a clock you know modulus arithmetic. The clock starts at 12 counts 1,2,3... and automatically resets at 12 and keeps going.

But ask any military person (me!) - the clock is 24 hours and starts at zero. So 12:35 am is "zero thirty-five hours". 00:00 is exactly midnight.

Like the military, the programming world counts from zero. Modulus arithmetic works because we start with zero. For programming generally we avoid oodles of "off by 1" errors by starting with zero.


CaesarCipher details

.

    public class CaesarCipher {
       public CaesarCipher( int encryptionKey );
       public string encrypt ( string clearText ) { ... }
       public string decrypt ( string secretText ) { ... }
    } 

I leave implementation to you but here's the basics.

A is zero, Z is 25, etc. This is a fixed reference. I think a single Array would work. Read the documentation! Learn how to reference array elements and/or array element indexes.

Pass an offset value to the constructor. Add the offset to encrypt, subtract the offset to decrypt. Or vice versa! See the Wikipedia article

littleOrphanAnnieDecoderRing = new CaesarCipher(3);
string scrambled = littleOrphanAnnieDecoderRing.encrypt("X"); 
scrambled == "A";  // true

Feel The Force

  • Viewing the real-world problem as interacting objects is very important. We want to identify functionality. Delay specific algorithm implementation details as much as practical.

  • Note how OO is kinda "outside - in" approach while your code is definitely "inside - out"

  • Outside-in seems to result in compilable code from the beginning. Inside-out seems to require lots of complex code up front.

  • Designing data structures - the CaesarCipher class w/ an encapsulated Array lookup - simplifies the hell out of subsequent code. This is HUGE.

    • The revelation that the clock is actually 24 hours with modulus behavior. This "24 data structure" removes 12 o'clock ambiguity, removes the need for am/pm qualifiers, and will greatly simplify the calculation code itself. If we want a 12 hour clock we can start with (inherit) the 24HourClock class because it is a more fundamental structure in the Apple Store quadrant of the time telling universe.
\$\endgroup\$

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.