Beginner user registration and login program

Question

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();
 }
}

Answer 1

• 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.

Answer 2

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();
 }
}

• Loki Astari: In addition to your recursion point. The function main() is special and recursive calls are not allowed.

Answer 3

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().