C++ Caesar cipher - follow-up

Question

I asked another question the other day and decided to rewrite the code with some of the suggestions given and add functionality (instead of reading from a console I'm reading directly from a file) the code works.

Looking at it, I think the nested for loop on the encode function doesn't look that good. I tried using std::transform like the comment on the code. It worked when I used the console just to read a line, now with vectors couldn't make it work.

// Ceaser Cipher implementation

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


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

    for (std::string lines : str)
    {
        // std::transform(lines.cbegin(), lines.cend(), std::back_inserter(tempMsg), [&](char ch) -> char
        for (char &ch : lines)
        {
            if (ch == 'z' || ch == 'Z')
            {
                ch -= 25;
            }
            else if (isspace(ch)) {
                ch = ' ';
            }
            else
            {
                ch += shift;
            }
        }
        tempMsg.push_back(lines);
    }

    return tempMsg;
}

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


int main(int argc, char *argv[])
{
    int choice;
    std::cout << "What do you want to do? 1.Encrypt, 2.Decrypt: ";
    std::cin >> choice;


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


    std::ifstream inFile("testfile.txt");
    if (!(inFile.is_open()))
    {
        std::cout << "There was a problem with the file!";
        std::exit(EXIT_FAILURE); // ExitProcess() if windows especific.
    }

    std::vector<std::string> finalResult;

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


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

    if (choice == 1)
    {
        auto result = encode(lines, key);
        finalResult = result;
    }

    else if (choice == 2)
    {
        auto result = decode(lines, key);
        finalResult = result;
    }
    else
    {
        std::cout << "Wrong option, exiting!";
        std::exit(EXIT_FAILURE);
    }


    std::ofstream outFile("testfile.txt");
    for (auto i = finalResult.begin(); i != finalResult.end(); ++i)
    {
        outFile << *i << '\n';
    }
    outFile.close();

    std::exit(EXIT_SUCCESS);
}

Answer 1

In the following, I am omitting checking for the correctness of the outputs as you have not provided a specification for what that should be. Nevertheless, consider the following points.

• I think the signatures of your encoding and decoding functions are a bit unexpected: I would expect an encoding function to take a single string and not a vector of strings. This gives me the feeling that the function is doing too much (i.e., violating the guideline of one function, one responsibility). So I would break the logic into smaller pieces:

  std::string encode(const std::string& str, int shift)
  {
      std::string str_enc;
      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
          {
              return ch + shift;
          }
      });
  
      return str_enc;
  }
  
  std::vector<std::string> encode(const std::vector<std::string>& str, int shift)
  {
      std::vector<std::string> tempMsg;
      std::transform(str.cbegin(), str.cend(), std::back_inserter(tempMsg),
          [&](const std::string& s) { return encode(s, shift); });
  
      return tempMsg;
  }
  

• In the main loop logic, before getting into any file reading, check whether the choice made by the user is sensible (either 1 or 2). This allows you to make finalResult const and to initialize it in a nicer way. This is a good modern practice as given by e.g., ES.28 of the C++ Core Guidelines:

  assert(choice == 1 || choice == 2);
  const std::vector<std::string> finalResult = [&]()
  {
      return choice == 1 ? encode(lines, key) : decode(lines, key);
  }(); 
  

• You don't have to explicitly close file streams. They close upon destruction.

• To check if the file is good to go, you can do if(inFile) { ... } invoking its operator bool() which tells you if the stream has no errors and is ready for I/O operations (specifically, it returns !fail()).

• When writing to file, you can use standard algorithms (like std::copy) since your logic is very simple:

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

• Everywhere in your code, avoid using std::exit (for reasons and more discussion, see here). Instead, just do return EXIT_FAILURE or return EXIT_SUCCESS as appropriate.

Answer 2

Take a look at your code, and think about the spec:

It should be, put all letters in order in a circle, if the input is in position n, the output is in position n + shift.

What you have is quite a lot more complicated, and in no way meets the spec.

---

1. You should add tempMsg.reserve(str.size()); to avoid reallocations.
2. You should move the line into tempMsg instead of making a spurious copy.
3. Consider normalising the shift before using it. And why not pre-calculate a lookup-table for use in the transform? Your choice on whether to do it on the fly, or do all 26 possible ones at compile-time.
4. There isn't actually any advantage to using std::transform instead of a for-range-loop, but anyway the third argument should be lines.begin().
5. Your naming is curious: str for all lines, lines for a single line... Also, why not call tempMsg something Shorter but more descriptive like ret?
6. You might want to reconsider your decision to read the whole file into memory before starting processing. It isn't even useful to read it as separate lines instead of blocks.