Implementation, Ceaser Cipher in C++

Question

I'm working on learning C++ coming from python. I'm doing some projects to help me out understanding the language a bit more.

My code works I just want to know if there is anything that I can improve on it or best practices in C++, maybe something that I should have done differently.

// Ceaser Cipher implementation

#include <iostream>
#include <string>




char shiftF(char ch, int shift, const char type)
{
    if (type == 'e')
    {
        return ch == 'z' || ch == 'Z' ? ch - 25 : ch + shift;
    }
    else if (type == 'd')
    {
        return ch == 'z' || ch == 'Z' ? ch - 25 : ch - shift;
    }
}


std::string encode(std::string str, int shift, char type)
{
    std::string tempMsg;
    for (auto ch : str)
    {
        if (isalpha(ch))
        {
            tempMsg += shiftF(ch, shift, type);
        }
        else if (isspace(ch))
        {
            // There is probably a better way to do this.
            tempMsg += " "; 
        }
        else if (isalnum(ch))
        {
            continue;
        }
    }
    return tempMsg;
}


int main(int argc, char *argv[])
{
    int choice;
    std::cout << "What do you want to do? 1.Encrypt, 2.Decrypt: ";
    std::cin >> choice;
    std::string result;
    const char dec = 'd';
    const char enc = 'e';

    if (choice == 1)
    {
        int key;
        std::cout << "Enter encryption shift: ";
        std::cin >> key;

        std::string msg;
        std::cout << "Enter a message: ";

        // This doesn't work for some reason?
        // std::cin.clear();
        // std::cin.sync();

        std::cin.ignore();
        std::getline(std::cin, msg);

        result = encode(msg, key, enc);
    }

    else if (choice == 2)
    {
        int key;
        std::cout << "Enter decryption shift: ";
        std::cin >> key;

        std::string msg;
        std::cout << "Enter a message: ";

        std::cin.ignore();
        std::getline(std::cin, msg);

        result = encode(msg, key, dec);
    }
    else
    {
        std::cout << "Wrong option, exiting!";
    }

    std::cout << "Message encoded: " << result;
}

Answer 1

Your functions are doing too much. Remember: one function, one responsibility. That is, the encode function actually both encodes and decodes and that behavior is controlled via its arguments. In particular, the choice of behavior is offloaded to shiftF based on a char. There is no error if that char is anything other than 'd' or 'e'. This is something that we should absolutely try to avoid: we want to catch as many problems as we can at compile-time, but even runtime errors are preferred to silent errors!

On closer inspection, there is no need for such a complication. In fact, the only difference between encoding and decoding is the sign of shift. So we could write:

std::string encode(const std::string& str, int shift)
{
    std::string tempMsg;

    std::transform(str.cbegin(), str.cend(), std::back_inserter(tempMsg), [&](char ch) -> char
    { 
        if (isspace(ch))
            return ' ';

        return ch == 'z' || ch == 'Z' ? ch - 25 : ch + shift; 
    });

    return tempMsg;
}

std::string decode(const std::string& str, int shift)
{
    return encode(str, -1 * shift);
}

What is happening here?

• We pass the input message str as a const-ref and not by-value as in your original code. When you pass by-value, the object gets copied and in this case there is no reason for it. In this case, you could actually even pass the object by-reference only, and modify it in-place but we're not doing it here.

• We use a standard function std::transform with a lambda function that encapsulates the logic of shiftF. You need to include <algorithm> for this use.

• The third argument for std::transform is std::back_inserter found from <iterator>, which takes care of inserting at the back of the string tempMsg.

• Conceptually, the division of encoding and decoding into separate functions is cleaner and more logical. Whenever you can implement another function in terms of other functions, it's likely a good idea because you don't have to repeat yourself leading to less maintenance decreasing the chances for bugs.

Once you adopt this approach, you can get rid of the const char variables from your main program, and just call the correct encode or decode function inside your if-statement. As a side remark, you could also return meaningful error codes in your main. For example, if the choice is invalid, return from the else-branch e.g., EXIT_FAILURE.

Answer 2

The code makes the assumption that the runtime character set encodes letters contiguously in order. Whilst this may be true on your system, it will fail on platforms that use EBCDIC or other codings with gaps between letters. It will also fail for codings such as IS 8859, which have letters outside of a..z,A..Z (e.g. à).

We're missing the include of <cctype> which defines std::isalpha, std::isspace and std::isalnum (all of which are misspelt in the code).

When reading input, always have a plan for what happens if the read fails. So std::cin >> key needs to be if (std::cin >> key) with appropriate code in the if and else branches.

Consider working as a filter for standard input, rather than operating on just a single line.

Answer 3

Not sure you ceaser cipher is working as expected:

return ch == 'z' || ch == 'Z' ? ch - 25 : ch + shift;

There is a special case for z or Z. This is not symmetric so decoding an encoded 'Z' or 'z' will fail. Also letters can be encoded as punctuation (which is why your encode() does not even try to enocde punctuation. I would fix this so that letters are encoded as letters (everything else is unencoded).

I would change your encoding to:

// int min:   Pass 'a' for std::islower() and 'A' for std::isupper()
// int dir:   Pass 1 for encode and -1 for decode.
char code(char ch, int shift, char dir, int min)
{
    return (ch + min + (dir * shift) % 26) + min;
}

Now that letters are encoced into other letters only (and don't splash out in into the punctuation range). We can simplify the shift function. It will handle letters and all other characters are left unencoded.

char shiftF(char ch, int shift, char dir)
{
    if (std::isalpha(ch) && std::islower(ch)) {
        return code(ch, shift, dir, 'a');
    }
    else if (std::isalpha(ch) && std::isupper(ch)) {
        return code(ch, shift, dir, 'A');
    }
    return ch;
}

As the shift now handles all letters correctly we don't need to any real work in the encode. But to make it safer and stop a sensitive string from being leaked around memory lets us change it to update the string in place.

void encode(std::string& str, int shift, char dir)
{
    for (auto& ch : str) {
        ch = shiftF(ch, shift, dir);
    }
}