Caesar cipher implementation in C++

Question

I've implemented a Caesar cipher in C++ and would like for you to review it. It works as expected, at least with the inputs I've tried.

Feel free to suggest improvements, warn about bad practices or mistakes that I might be doing or give general tips.

// Caesar Cipher implementation

#include <iostream>
#include <string>
#include <algorithm>
#include <fstream>
#include <vector>
#include <cassert>
#include <iterator>


std::string encodeStr(const std::string& str, int shift, bool decodeFlag = false)
{
    std::string str_enc;
    // not sure if this function is doing to much
    std::transform(str.cbegin(), str.cend(), std::back_inserter(str_enc), [&](char ch) -> char
    {
        if (ch == 'z' || ch == 'Z')
        {
            return ch - 25;
        }
        else if (isspace(ch))
        {
            return ' ';
        }
        else
        {
            if (islower(ch))
                ch ^= 0x20; // XOR 6th bit don't care if it's lower case.
            if (decodeFlag)
            {
                return ((ch - 'A' + 26) - shift) % 26 + 'A';
            }
            else
            {
                return ((ch - 'A') + shift) % 26 + 'A';
            }
        }
    });
    return str_enc;
}

std::vector<std::string> encodeVec(const std::vector<std::string>& strVector, int shift, bool decodeFlag = false)
{
    std::vector<std::string> tempMsg;
    tempMsg.reserve(strVector.size());

    std::transform(strVector.cbegin(), strVector.cend(), std::back_inserter(tempMsg),
        [&](const std::string& s) { return encodeStr(s, shift, decodeFlag); });

    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;

    if (!(choice == 1 || choice == 2)) {
        return EXIT_FAILURE;
    }

    int key;
    std::cout << "Enter desired shift: ";
    std::cin >> key;


    std::ifstream inFile("test.txt"); // Might ask for the file instead of hard coding it...
    if (!inFile)
    {
        std::cout << "There was a problem with the file!";
        return EXIT_FAILURE;
    }

    std::string line;
    std::vector<std::string> lines;

    while (std::getline(inFile, line))
    {
        lines.push_back(line);
    }

    const std::vector<std::string> finalResult = [&]()
    {
        return choice == 1 ? encodeVec(lines, key) : encodeVec(lines, key, true);
    }();

    std::ofstream outFile("test.txt");
    std::copy(finalResult.cbegin(), finalResult.cend(),
        std::ostream_iterator<std::string>(outFile, "\n"));

    return EXIT_SUCCESS;
}

Answer 1

You can accept arguments from the command line.

You don't need to differentiate between decode and encode, you can simply provide negative shift.

You can hide the big lambda into a char -> char function.

You actually don't need the string -> string nor vector -> vector implementations.

encodeStr is a terrible name :)

You can leverage the char -> char implementation and copy istream_iterator<char> to ostream_iterator<char> char by char, and doing it over std::cin and std::cout will give you the ability to change the files as needed (through pipes) or use standard input / output by default.

#include <iostream>
#include <algorithm>
#include <iterator>
#include <sstream>

const unsigned char caesar_range = 'Z' - 'A' + 1;

char caesar_cipher(char c, int shift)
{
    char offset;
    if (c >= 'a' && c <= 'z') offset = 'a';
    else if (c >= 'A' && c <= 'Z') offset = 'A';
    else return c;

    shift = shift % caesar_range;
    auto base = c - offset + shift;
    if (base < 0) {
        base = caesar_range + base;
    }
    return base % caesar_range + offset; 
}

void caesar_usage()
{
    std::cout << "Usage caesar <shift>\n";
    std::cout << "\tshift must be integer and providing -shift decodes string encoded with shift.\n";
}

int main(int argc, char *argv[])
{
    if (argc != 2) {
        caesar_usage();
        return EXIT_FAILURE;
    }

    int shift;
    std::stringstream ss(argv[1], std::ios_base::in);
    ss >> shift;
    if (ss.fail() || !ss.eof()) {
        caesar_usage();
        return EXIT_FAILURE;
    }

    // turn off white space skipping
    std::cin >> std::noskipws;

    std::transform(
        std::istream_iterator<char>(std::cin),
        std::istream_iterator<char>(),
        std::ostream_iterator<char>(std::cout), 
        [&](char c) -> char {
          return caesar_cipher(c, shift);
        }
    );

    return EXIT_SUCCESS;
}

Encode | decode: ./caesar 5 <in.txt | ./caesar -5

Answer 2

1. You should really look into the algorithm. If you do, you will see that encoding and decoding are essentially the same operation, though with negated key.

2. Avoid passing a std::string or the like by constant reference. Accepting a std::string_view by value is generally more flexible and efficient.

3. .reserve() enough space at the start, and you won't have any wasteful reallocations.

4. Always test with a few trivial examples to weed out brokenness. In your case, z and Z are broken for 25 of the 26 possible keys for each direction.

5. Use auto to avoid repeating types, and potentially getting a costly mismatch.

6. Consider using an in-place transformation instead.

Answer 3

Bug

Your encodeStr() method takes a shift, and a decodeFlag, but when it encounters a 'z' or a 'Z', it ignores both and returns ch - 25. This means any string which contains a Z or encodes to contain a Z will not be decodable.

Answer 4

Alternative

I would use an alternative technique:

Since your application basically reads a file and encodes it, why not simply make the stream do the encoding?

I would do something like this:

#include <locale>
#include <algorithm>
#include <iostream>
#include <fstream>

int shift = 0;

class CaesarCipher: public std::codecvt<char,char,std::mbstate_t>
{
  protected:
    virtual std::codecvt_base::result
        do_out(state_type& tabNeeded,
                         const char* rStart, const char*  rEnd, const char*&   rNewStart,
                         char*       wStart, char*        wEnd, char*&         wNewStart) const
    {
        for(;rStart < rEnd;++rStart, ++wStart)
        {
            if (!std::isalpha(*rStart)) {
                *wStart = *rStart;
            }
            else {
                char input = *rStart | 0x20;
                *wStart = (input - 'A' - shift) % 26 + 'A';
            }
        }

        rNewStart   = rStart;
        wNewStart   = wStart;

        return std::codecvt_base::ok;
    }

    // Override so the do_out() virtual function is called.
    virtual bool do_always_noconv() const throw()  {return false;}
};

int main(int argc, char* argv[])
{
    if (argc != 2) {
        std::cerr << "Error Expected a shift value\n";
        exit(1);
    }
    shift = std::atoi(argv[1]);

    // Open the input file
    std::ifstream inFile("input.txt");

    // Create an output file.
    // Imbue it with the encoding facet
    // Open the file.
    std::ofstream outFile;
    outFile.imbue(std::locale(std::locale::classic(), new CaesarCipher()));
    outFile.open("output.txt");


    // Copy input file to output file
    outFile << inFile.rdbuf();
}

---

Code Review

This is horrible:

const std::vector<std::string> finalResult = [&]()
{
    return choice == 1 ? encodeVec(lines, key) : encodeVec(lines, key, true);
}();

Simply do:

const std::vector<std::string> finalResult = encodeVec(lines, key, choice != 1);

---

Why do you need a encode/decode version? They are doing literally the same thing. In most systems the encode and decode strings are different anyway. In this case the decode number is simply the negative of the encode number (or 26 - <encode number>).

        if (decodeFlag)
        {
            return ((ch - 'A' + 26) - shift) % 26 + 'A';
        }
        else
        {
            return ((ch - 'A') + shift) % 26 + 'A';
        }

I would simply write:

        return ((ch - 'A') + shift) % 26 + 'A';

Then have users use different values to encode/decode.

---

This:

if (islower(ch))
            ch ^= 0x20; // XOR 6th bit don't care if it's lower case.

Is just a complex way of doing:

ch |= 0x20; // Always add this flag to make everything upper case.

Note the encoding stuff is not going to work for any special characters (only uppercase letters) so treat everything as a character.

---

OK, so space is special.

    else if (isspace(ch))
    {
        return ' ';
    }

As everything that is not an uppercase letter (or converted to uppercase) is not going to decode properly then everything that is not a character should treated as special.

    else if (!std::isalpha(ch))
    {
        return c;
    }

---

Somebody else mentioned this is a bug:

    if (ch == 'z' || ch == 'Z')
    {
        return ch - 25;
    }

---

You do a lot of copying of strings to get this working.

Some thought about doing the encoding in-place may save you a lot.

---