Caesar cipher encryption

Question

I've been studying C++ (my first language) for about two months with books and Udemy courses. I would like some comments about my first real project that I put together for my teacher.

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

using namespace std;
class Caesar {

private:
int userChoice;
string encrypt;
char temp;
public:

Caesar() {};
int caesarInterface(Caesar *obj)
{
    system("CLS");
    cout << "===================================================" << endl;
    cout << "|            Welcome to Caesar's cipher           |" << endl;
    cout << "===================================================" << endl;
    cout << "|                                                 |" << endl;
    cout << "| 1. Encrypt a .txt file.                         |" << endl;
    cout << "| 2. Decrypt a .txt file.                         |" << endl;
    cout << "| 3. Exit program.                                |" << endl;
    cout << "|                                                 |" << endl;
    cout << "|                                                 |" << endl;
    cout << "|                                                 |" << endl;
    cout << "|                                                 |" << endl;
    cout << "|                                                 |" << endl;
    cout << "| A software writen by Ricardo Martins Doberstein |" << endl;
    cout << "===================================================" << endl;
    try
    {
        cin >> userChoice;
        if (obj->userChoice == 1) return obj->userChoice;
        if (obj->userChoice == 2) return obj->userChoice;
        if (obj->userChoice == 3) return obj->userChoice;
        else throw 199;
    }
    catch (int a)
    {
        cout << "No such choice... error: 199" << endl;
        cout << "Please try again." << endl;
        obj->userChoice = 3;
        system("pause");
        return obj->userChoice;
    }
}

void cesarEncrypt(Caesar *obj, string file)
{
    string tempString;
    vector<string> vec;
    ifstream encrypt;
    ofstream result;
    result.open("encrypted.txt");
    encrypt.open(file);
    try
    {
        if (encrypt.is_open() && result.is_open())
        {
            while (!encrypt.eof())
            {
                getline(encrypt, tempString);
                vec.push_back(tempString);
            }
            vector<string>::iterator it = vec.begin();
            for (it; it != vec.end(); it++)
            {
                tempString = *it;
                for (int i = 0; i < tempString.length(); i++)
                {
                    if (tempString[i] == ' ') tempString[i] = ' ';
                    else if (tempString[i] == '!') tempString[i] = '!';
                    else if (tempString[i] == '.') tempString[i] = '.';
                    else if (tempString[i] == ',') tempString[i] = ',';
                    else if (tempString[i] == 'x') tempString[i] = 'a';
                    else if (tempString[i] == 'y') tempString[i] = 'b';
                    else if (tempString[i] == 'z') tempString[i] = 'c';
                    else if (tempString[i] == 'X') tempString[i] = 'A';
                    else if (tempString[i] == 'Y') tempString[i] = 'B';
                    else if (tempString[i] == 'Z') tempString[i] = 'C';
                    else tempString[i] += 3;
                }
                result << tempString + '\n';
            }
            encrypt.close();
            result.close();
            cout << "A file (encrypted.txt) has been saved in this program's directory successfully" << endl;
        }
        else throw 299;
    }
    catch (int error)
    {
        cout << "Couldn't open files. Error: 299" << endl;
    }
}

void caesarDecrypt(Caesar *obj, string file)
{
    string tempString;
    vector<string> vec;
    ifstream decrypt;
    ofstream result;
    result.open("decrypted.txt");
    decrypt.open(file);
    try
    {
        if (decrypt.is_open() && result.is_open())
        {
            while (!decrypt.eof())
            {
                getline(decrypt, tempString);
                vec.push_back(tempString);
            }
            vector<string>::iterator it = vec.begin();
            for (it; it != vec.end(); it++)
            {
                tempString = *it;
                for (int i = 0; i < tempString.length(); i++)
                {
                    if (tempString[i] == ' ') tempString[i] = ' ';
                    else if (tempString[i] == '!') tempString[i] = '!';
                    else if (tempString[i] == '.') tempString[i] = '.';
                    else if (tempString[i] == ',') tempString[i] = ',';
                    else if (tempString[i] == 'a') tempString[i] = 'x';
                    else if (tempString[i] == 'b') tempString[i] = 'y';
                    else if (tempString[i] == 'c') tempString[i] = 'z';
                    else if (tempString[i] == 'A') tempString[i] = 'X';
                    else if (tempString[i] == 'B') tempString[i] = 'Y';
                    else if (tempString[i] == 'C') tempString[i] = 'Z';
                    else tempString[i] -= 3;
                }
                result << tempString + '\n';
            }
            decrypt.close();
            result.close();
            cout << "A file (decrypted.txt) has been saved in this program's directory successfully" << endl;
        }
        else throw 399;
    }   
    catch (int error)
    {
        cout << "Couldn't open files. Error: 399" << endl;
    }
}

~Caesar() {};
};

int main()
{
string filePath;
bool isRunning = true;
int userChoice;

Caesar *caesar = new Caesar;

while (isRunning)
{
    userChoice = caesar->caesarInterface(caesar);
    if (userChoice == 1)
    {
        cout << "Type the .txt file path." << endl;
        cin >> filePath;
        caesar->cesarEncrypt(caesar, filePath);
        system("pause");
        isRunning = false;
    }
    if (userChoice == 2)
    {
        cout << "Type the .txt file path." << endl;
        cin >> filePath;
        caesar->caesarDecrypt(caesar, filePath);
        system("pause");
        isRunning = false;
    }
    if (userChoice == 3) isRunning = false;
    }
    return 0;
}

Answer 1

What is system("CLS"), system("pause")? That is not C++.

Your top function uses exceptions for normal flow control, and is completely unnecessary.

try
{
    cin >> userChoice;
    if (obj->userChoice == 1) return obj->userChoice;
    if (obj->userChoice == 2) return obj->userChoice;
    if (obj->userChoice == 3) return obj->userChoice;
    else throw 199;
}
catch (int a)
{
    cout << "No such choice... error: 199" << endl;
    cout << "Please try again." << endl;
    obj->userChoice = 3;
    system("pause");
    return obj->userChoice;
}

You also duplicate the entire code for each case when they are identical.

Now this is weird and I don’t follow: you read into userChoice with no qualification or receiver on that name. But you then use the value from obj->userChoice which is a different variable.

OK, here is the only call to it:

userChoice = caesar->caesarInterface(caesar);

You are passing the same object as this and obj. You are doing the same, I see, with ceasarEncrypt and ceasarDecrypt.

I think you don’t understand how member functions work, which is a more serious problem.

Anyway, ignoring obj from now on,

This should not be a member at all, but part of the program that uses the class.

It should use conditionals rather than repetitive code. E.g.

cin >> choice;
if (choice < 1 || choice > 3) {
    cout << "usage notes\n"
    return 3;
return choice;

---

Likewise, your encrypt and decrypt functions are nearly identical. I suggest moving the file opening/closing out to a higher level, and combine the encrypt/decript logic into one function that parameterizes the only slight difference between them.

        vector<string>::iterator it = vec.begin();
        for (it; it != vec.end(); it++)
        {
            tempString = *it;

You don’t know about the range-for loop. It is pretty cool, and exactly what you need here:

    for (const auto& tempString : vec) { ⋯

that is, it makes an iterator and dereferences it into a loop variable for you.

            for (int i = 0; i < tempString.length(); i++)
            {
                if (tempString[i] == ' ') tempString[i] = ' ';
                else if (tempString[i] == '!') tempString[i] = '!';
                else if (tempString[i] == '.') tempString[i] = '.';
                else if (tempString[i] == ',') tempString[i] = ',';
                else if (tempString[i] == 'x') tempString[i] = 'a';
                else if (tempString[i] == 'y') tempString[i] = 'b';
                else if (tempString[i] == 'z') tempString[i] = 'c';
                else if (tempString[i] == 'X') tempString[i] = 'A';
                else if (tempString[i] == 'Y') tempString[i] = 'B';
                else if (tempString[i] == 'Z') tempString[i] = 'C';
                else tempString[i] += 3;
            }

I thought about removing some of the left margin when posting, but then realized that this is another rule of thumb to point out: do you see how deep your function blocks are getting? That is a sign that your function is doing too much, and should use more Top-Down Decomposition.

The use of [i] to index each position of the string is not the usual way to do things in C++. Anyway, see how many times you use tempString[i]? Did you think about some way of doing the subscripting once only at the top of the loop?

The three punctuation marks are unchanged, but what about all other marks? I think you really mean to pass everything that’s not a letter.

The cases for x,y,z, which are the ones that wrap, is kind of clever. But again you list each line in full thee times instead of using a conditional to specify all the cases and one thing to do with those.

But really, what you want here is modulo. And don’t be afraid of (more) Top-Down Decomposition! So a first sketch becomes:

for (auto& ch : tempString) {
    if (ch >= 'a' && ch <= 'z')  rotate (ch, 'a', 3);
    else if (ch >= 'A' && ch <= 'Z')  rotate (ch, 'A', 3);
    // else ch is not changed.
}

So, isolate the two groups of letters and do the same thing to each range, where I pushed how to do that down into another function I’ll write later. That rotate function needs the value to work on, needs to know how the two ranges (upper and lower) differ so it work on both, and I also parameterized just how for to rotate — this allows 3 to encode and −3 to decode, all in the same function.

So, this function can encode/decode too, and should take a similar parameter which is passes through.

Following the earlier note to break it up from the file I/O, and here I also separate it from slurping in the whole file. The one thing this function does it work on a single string. It does not care where it came from or what will happen to it. It does one thing only: rotate the letters in a string.

void ceasar (std::string& s, int shift)
{
    for (auto& ch : s) {
        if (ch >= 'a' && ch <= 'z')  rotate (ch, 'a', shift);
        else if (ch >= 'A' && ch <= 'Z')  rotate (ch, 'A', shift);
        // else ch is not changed.
    }
}

that’s it!

A higher function does the file reading and writing, and a lower function does the rotate logic.

As for the file reading, you are doing the exception thing again: throwing 299 and catching it again in the same function, where there was no need for non-local flow control in the first place.

For the rotate, we want to add the shift but wrap around. If you start with zero, that is a simple modulo operation. So, subtract the starting letter to get a number from 0 through 25 inclusive.

void rotate (char& ch, char base, int shift)
{
    int code= ch-base;
    code = (code+shift)%26;
    ch= code+base;
}

Also note that the ceasar function has nothing to do with any class. It stands alone, and does not have any instance data. So don’t make it a class.

---

string filePath;
bool isRunning = true;
int userChoice;

Put your variables where they are first used, and ready to be initialized. Don’t put them all clustered at the top of the function.

Caesar *caesar = new Caesar;

If you did need an object, there is no need to allocate it on the heap. Unlike certain other languages that came later, C++ lets you declare variables as values which are allocated on the stack frame of the function. So just

Caesar caesar;

whould be what you use.

The body of main has almost identical blocks for choice 1 and choice 2. The only difference is the call to Encrypt vs Decrypt. In the updated code above, this would be a difference of calling the same function with a 3 or a −3. In any case, write the block once, and use an if statement or the like only for the part that is different.

---

Hope that helps with some of the deeper understanding. Good luck, and keep at it!

Answer 2

• Don't use using namespace std;.
• There is no need to declare class members private if they appear at the top of your class. However it is customary to declare public members first so that users (yourself included) can see what they can work with without having to read through class internals.
• Don't declare an empty constructor or destructor. If you are going to then at least declare it = default.
• system("CLS") is non-standard and non-portable. It is generally considered bad-practice to call into the OS unless you have good reason too.
• In C++ it is customary to put the identifier with the type. ie Caesar* obj as opposed to the variable.
• Don't use new or delete. If you are going to use pointers then you should probably started using smart pointers. However.
• Don't use pointers when you don't need them. In your main you absolutely could have called Caesar caesar;.
• Why do your function calls require you to pass the object back to itself? any information that the instance contains is available to itself at any time. That includes private member data.

• Using .eof() the way you do produces a bug. The loop will iterate one more time after the end of file is reached. This is because eof() does not get set to true until it tries to read past the end of the file. I found this snippet when searching for a better way to do this:

  while(file.peek() != EOF)
  

• avoid using std::endl. This performs a flush of the stream which you typically don't want or need and can cause performance issues. \n is sufficient and adding in a std::flush if you need to flush the stream will make it clear you meant to do it.