8
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I am just going to be a nitpick, but the ceasar cipher is a specifc case of a shiftcipher. What you have here is a shiftcipher, while a ceasarcipher would be a shiftcipher with shift=3. A ceasar cipher would make your implementation a lot easier ;) \$\endgroup\$ – Danagon Nov 14 at 12:45
  • 1
    \$\begingroup\$ @Danagon Really? Even Wikipedia gives examples with a shift of 25 \$\endgroup\$ – Exzlanttt Nov 14 at 13:29
  • 1
    \$\begingroup\$ It was three ... \$\endgroup\$ – Mawg Nov 14 at 13:54
  • 1
    \$\begingroup\$ @Exzlanttt It is commonly misused (to the point that you may even consider it correct today like sometimes happens to language) by people, not into cryptography. The Ceasar cipher is named after Julius Ceasar, who always used a shiftcipher with shift=3 to encode important briefs. That's why the ceaser cipher specifically should use a shift of three and there is a more general name (shiftcipher) for the broader scheme. Which in turn is a specific case of the monoalphabetic substitution cipher, and so on. \$\endgroup\$ – Danagon Nov 14 at 14:22
  • 1
    \$\begingroup\$ @Danagon I'm going to nitpick too...you spelled "Caesar" wrong every time. Plus, Caesar's nephew used a shift of ONE. So, I'd say that it doesn't matter. \$\endgroup\$ – Casey Nov 14 at 14:42
8
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ You can use {} as the second argument to transform to save on characters :) \$\endgroup\$ – Rakete1111 Nov 14 at 9:13
  • \$\begingroup\$ In what way is encodeStr a terrible name? Can you elaborate (by editing your answer, not here in comments)? \$\endgroup\$ – Peter Mortensen Nov 14 at 12:05
5
\$\begingroup\$
  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.

\$\endgroup\$
  • 2
    \$\begingroup\$ I know the Z part is broken that if shouldn't exist. Didn't knew about string_view, thank you! \$\endgroup\$ – Exzlanttt Nov 13 at 15:59
  • 1
    \$\begingroup\$ std::string_view is for non-mutating operations and will not work with std::transform when the destination is the string_view. Since encryption/decryption implies mutation, I would recommend taking the std::string by value since a copy is being made regardless if using std::string_view or not. \$\endgroup\$ – Casey Nov 14 at 16:45
  • 1
    \$\begingroup\$ @Casey In-place encryption/decryption would imply mutation, yes. And that's one of my suggestions. But currently the code does not do in-place, and if it doesn't the view is better than the constant reference. \$\endgroup\$ – Deduplicator Nov 14 at 17:27
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Yea that is from an earlier version without the flag forgot to delete that part and didn't test for strings with a 'Z'! Thank you for spotting it \$\endgroup\$ – Exzlanttt Nov 13 at 14:56
  • 2
    \$\begingroup\$ "The Quick Brown Fox Jumped Over The Lazy Dog" is your friend. :-). Add a test case encoding, and subsequently decoding it, and verifying the original string is returned. \$\endgroup\$ – AJNeufeld Nov 13 at 15:01
  • 2
    \$\begingroup\$ Property-based tests are even more your friend! Just test that encode(str, a + b) = encode(encode(str, b), a) whenever 0 <= a,b < 26, and that encode(str, 26) = str, and you're 7/8 of the way to knowing that you're correct without even looking at any code. \$\endgroup\$ – Patrick Stevens Nov 14 at 7:33
3
\$\begingroup\$

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.


\$\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.