2
\$\begingroup\$

I'm new to coding and C++, this is one of my first pieces of "software". Could anyone give me some constructive criticism?

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

using namespace std;

bool IsLoggedIn()
{
    string user, pass, un, pw;
    cout << "Username: " << endl;
    cin >> user;
    cout << "Password: " << endl;
    cin >> pass;

    ifstream read(user + ".txt");
    getline(read, un);
    getline(read, pw);

    if (un == user && pw == pass)
        return true;
    else
        return false;

}

inline bool does_file_exist(const string& name)
{
    ifstream filename(name +".txt");
    if(!filename)
        return false;
    else
        return true;
}

int main()
{
    system("clear");

    int input;
    cout << "(1) Create Account \n" << "(2) Log in \n"<< "(3) Change password" << endl;
    cin >> input;
    if (input == 1 || input == 2 || input == 3)
    {
        if (input == 1)
        {
            system("clear");

            string user, pass;

            cout << "Create Account \n" << endl;
            cout << "Username: " << endl;
            cin >> user;

            if (does_file_exist(user))
            {
                cout << "Username already registered \n" << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
            }

            cout << "Password: " << endl;
            cin >> pass;

            ofstream file;
            file.open(user + ".txt");
            file << user << endl << pass;
            file.close();

            main();
        }

        else if (input == 2)
        {
            system("clear");

            cout << "Log In \n" << endl;

            bool status = IsLoggedIn();

            if (!status)
            {
                cout << "Incorrect username or password!" << endl << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
                return 0;
            }
            else
            {
                cout << "Successfully logged in!" << endl;
                cin.get();
                return 1;
            }

        }

        else if (input == 3)
        {
            system("clear");

            string user, pass;

            cout << "Change password \n" << endl;
            cout << "Username: " << endl;
            cin >> user;
            if (does_file_exist(user))
            {
                cout << "New Password: " << endl;
                cin >> pass;

                ofstream file;
                file.open(user + ".txt");
                file << user << endl << pass;
                file.close();

            main();
            }
            else
            {
                cout << "Username not registered \n" << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
            }

        }
    }
    else
    {
        system("clear");
        cout << "Invalid input" << endl;
        main();
    }
}
\$\endgroup\$
2
\$\begingroup\$
  • Never ever recurse into main. You may get away with it in C; in C++ it would surely wake dragons cause undefined behavior. Use loops.

  • There is no need to validate input separately.

        if (input == 1) {
            ....
        } else if (input == 2) {
            ....
        } else if (input == 3) {
            ....
        } else {
            std::cout << "Invalid input\n";
        }
    

    is sufficient. As a side note, instead of cascading ifs, consider a switch statement.

  • Avoid magic numbers. Define few symbolic constants, e.g.

    static const int Register = 1;
    // etc
    

    and use them.

  • DRY.

    First of all, system("clear") is called in all branches. Call it once. As a side note, avoid calling it at all. It may not be present in a target system.

    Second, the repeated code such as

        ofstream file;
        file.open(user + ".txt");
        file << user << endl << pass;
        file.close();
    

    shall be factored into a function.

  • does_file_exist does not have a right to exist. It introduces a time-to-check/time-to-use race condition. Just open a file and see if it opened successfully. For example,

        ofstream file(user + ".txt");
        if (file) {
            proceed();
        } else {
            report_error();
        }
    

    As a side note,

        if (!filename) {
            return false;
        } else {
            return true;
        }
    

    is a very long way to say

        return filename;
    

    Ditto for IsLoggedIn.

  • Be consistent with the return values. Some of the branches do return, some does not. As yet another side note, traditionally returning 0 means success.

\$\endgroup\$
  • \$\begingroup\$ Noted. Thank you! <3 \$\endgroup\$ – TheClassicSmoke Aug 12 at 19:29
2
\$\begingroup\$

Not a full review, but too long for a comment

I'm not a C++ developer, but there are a few common foundational programming mistakes that you've made that I saw:

  • using namespace std is a bad practice. This imports the entirety of the namespace into the current namespace, which can cause a multitude of problems. Here is an article that explains the possible problems, going more in-depth than I could ever. *Note: I didn't change anything that would require std:: at the beginning by removing the namespace, merely for the fact that I don't have the time right now. That can be a good exercise for you :).
  • Instead of return true; } else { return false; }, simply return the expression you're evaluating. For example in your code, return (un == user && pw == pass).
  • When a user enters an already existing username/incorrect username or password/username not registered, you call main() again. This can fill the stack pretty quickly with these recursive calls. What if a user keeps making mistakes? Then you have tens or even hundreds of main calls in the stack. Not good. You can wrap all the code in main in a while loop. If a user makes a mistake, simply do nothing and let the loop reset. If a user makes a valid choice, do what you need to do, then break. Note: Didn't implement this either because of time, this one is for you :).

Barely changed but still changed code

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

//ADD "std::" TO ALL THINGS THAT COME FROM STD NAMESPACE

bool IsLoggedIn()
{
    string user, pass, un, pw;
    cout << "Username: " << std::endl;
    cin >> user;
    cout << "Password: " << endl;
    cin >> pass;

    ifstream read(user + ".txt");
    getline(read, un);
    getline(read, pw);

    return (un == user && pw == pass);

}

inline bool does_file_exist(const string& name)
{
    ifstream filename(name +".txt");

    return filename;

}

int main()
{
    system("clear");

    int input;
    cout << "(1) Create Account \n" << "(2) Log in \n"<< "(3) Change password" << endl;
    cin >> input;
    if (input == 1 || input == 2 || input == 3)
    {
        if (input == 1)
        {
            system("clear");

            string user, pass;

            cout << "Create Account \n" << endl;
            cout << "Username: " << endl;
            cin >> user;

            if (does_file_exist(user))
            {
                cout << "Username already registered \n" << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
            }

            cout << "Password: " << endl;
            cin >> pass;

            ofstream file;
            file.open(user + ".txt");
            file << user << endl << pass;
            file.close();

            main();
        }

        else if (input == 2)
        {
            system("clear");

            cout << "Log In \n" << endl;

            bool status = IsLoggedIn();

            if (!status)
            {
                cout << "Incorrect username or password!" << endl << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
                return 0;
            }
            else
            {
                cout << "Successfully logged in!" << endl;
                cin.get();
                return 1;
            }

        }

        else if (input == 3)
        {
            system("clear");

            string user, pass;

            cout << "Change password \n" << endl;
            cout << "Username: " << endl;
            cin >> user;
            if (does_file_exist(user))
            {
                cout << "New Password: " << endl;
                cin >> pass;

                ofstream file;
                file.open(user + ".txt");
                file << user << endl << pass;
                file.close();

            main();
            }
            else
            {
                cout << "Username not registered \n" << "Press enter to retry...";
                cin.ignore();
                cin.get();
                main();
            }

        }
    }
    else
    {
        system("clear");
        cout << "Invalid input" << endl;
        main();
    }
}
\$\endgroup\$
  • \$\begingroup\$ In addition to your recursion point. The function main() is special and recursive calls are not allowed. \$\endgroup\$ – Martin York Aug 13 at 0:56
0
\$\begingroup\$

I agree with all of the other reviews but I would recommend you to use a a switch statement instead of long if-elses.

switch(input){
case 1:
// do stuff
break;
case 2:
// do other stuff
break;
case 3:
// do other stuff
break;
default:
// Invalid input
break;
}

In my opinion this increases readability and provides a good way of checking states and catching invalid inputs.

To clear the console screen your are using system("clear") which is okay, but not good practise because its OS-specific. For Unix based systems like MacOS and Linux this would cause undefined behaviour or simply do nothing. A workaround that is portable and can be used on *NIX platforms is simply using ANSI escape characters:

void clear() {
    // CSI[2J clears screen, CSI[H moves the cursor to top-left corner
    std::cout << "\x1B[2J\x1B[H";
}

(Taken from User Cat Plus Plus from this thread https://stackoverflow.com/questions/6486289/how-can-i-clear-console)

Now you have to options, either determine which OS the program runs or just trying both system(clear) and clear().

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