2
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ Please edit your post to include your test cases. The test cases should contain at least one test that shifts the complete alphabet by 7 places and unshifts it again. Another test should demonstrate what happens when you encode a phone number by shifting it 32 places and then decode it again. (I expect these tests to fix the worst bugs.) As long as there are no answers yet, you may still update the post. \$\endgroup\$ – Roland Illig Mar 10 '19 at 8:00
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

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.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hi, about point 5. You are rigth I forgot to change variables when I went from reading just one string to reading files. Point 6 - do you mean read as I do the encoding? Point 3 - What do you mean by lookup-table? Anyway thank you for your help! \$\endgroup\$ – Exzlanttt Mar 10 '19 at 15:40
  • \$\begingroup\$ re 6: Yes, no need for any dynamic allocation. re 3: Simply a unsigned char[1 + (unsigned char)-1] where you pre-compute the result for every input, making the transformation itself trivial. And as there are only 26 variations for that lookup-table, it might make sense to calculate them all at compile-time and choose. \$\endgroup\$ – Deduplicator Mar 10 '19 at 16:13

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.