2
\$\begingroup\$

This is my first program i write after i finished the c++ tutorials, that program can login and register,

the program ask the user to choice login or register after user choice and write his username the program will choice if this user already exist or not than will take him to next step to write the password and confirm it, after that the program will save the username and password in "txt" file. the login will read the username and password from the same file...

i feel i did a lot of code duplication special on read or write from the files.txt i think should i use classes&objects but i don't know how i feel write The code scattered and disorganized.

My question guys can you give me some advices to improve my code. something should i working on ??

// Note Please you need to create a *users.txt* files before run the program
#include <iostream>
#include <fstream>
#include <sstream>
using namespace std;


//Login function
void login(){
string UserName,Password;
string UseEnter,PassEnter;
bool On_Off = true;
bool Off = true;
int countt = 0;
int left = 3;

cout << "\n**** Login to your Account **** \n";
while(On_Off){
    //********************************************
    fstream user;
    user.open("users.txt", ios::in);
    if(countt < 3 && Off && user.is_open()){
        if(countt > 0){
        cout << "\nPassword or UserName are not correct!!" << endl;
        left--;
        cout << "You have *" << left << "* attempts left \n" << endl;
        }
        cout << "Enter UserName: ";
        getline(cin,UseEnter);
        cout << "Enter Password: ";
        getline(cin,PassEnter);
        countt++;
        string read;
        //====================================================*
        while(getline(user,read)){
            stringstream convertor(read);
            convertor >> UserName >> Password;
            if(UseEnter == UserName && PassEnter == Password){
                Off = false;}}
        //=====================================================*
            }
    else if(!Off){
        cout << "\n**** Welcome! ****\n\n";
        user.close();
        On_Off = false;
}
    else {
        cout << "\n**** try again later!! **** \n\n";
        user.close();
        On_Off = false;
    }
}
}

// Registry function
void registry(){
string newName,checkName,newPass,confirmPass;
int MiniPass ;
bool if_ON = true;
bool oo = true;

cout << "\n**** Create New Account **** \n";

//UserName Validation
while(oo){
    cout << "Enter New UserName: ";
    getline(cin, newName);
    MiniPass = newName.length();
    fstream yusers;
    yusers.open("Users.txt",ios::in);
    if(if_ON && MiniPass >= 1 && yusers.is_open()){
        string readd;
        while(getline(yusers,readd)){
            stringstream OnlyUsers(readd);
            OnlyUsers >> checkName;
            if(newName == checkName){
                cout <<"\n*"<< newName <<"* UserName already exists Try another one\n" << endl;
                if_ON = false;
            }
    }
    yusers.close();
    if(!if_ON){
       if_ON = true;
    }
    else{
        oo = false;
    }
    }
}
//Password Validation
while(true){
    cout << "Enter New password: ";
    getline(cin,newPass);
    cout << "Confirm the password: ";
    getline(cin,confirmPass);
    MiniPass = newPass.length();

    if (MiniPass < 5){
        cout << "\nYour password is weak at least 5 letters\n \n";
    }
    else if(confirmPass == newPass){
        fstream users;
        users.open("Users.txt",ios::app);
        if(users.is_open()){
            users << newName +" "+ newPass << endl;
            users.close();
        }
        cout << "\n===================================\n";
        cout << "\nSuccessfully new account created! \n \n";
        cout << "===================================\n";
        break;}
    else{
        cout << "\nPassword confirmation not correct! \n" << endl;

        }
        }
    login();

}




int main()
{
//User can choice Login or register by writing <login> or <register>
string LoR;
cout << "\n***** Login or Register *****\n\n";
cout << "===================================\n";
cout << "\n***** Please write (login) or (register) *****\n\n";

while(true){

    getline(cin,LoR);
    if(LoR == "login"){
        login();
        break;
    }else if(LoR == "register"){
        registry();
        break;
    }else{
        cout << "\n*Please write (login) or (register) correctly*\n" ;
    }
}
return 0;
}
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Please indent your code

Taking a first pass it was really difficult to determine where functions ended. While I was scanning it looked like you have random while statements outside your functions. Indentation helps readability a lot, so please do so.

Spaces after keywords / Spaces before & after operators

while() -> while ()
if() -> if ()
for() -> for ()
cout <<"\n*"<< -> cout << "\n*" <<
+" "+ -> + " " +

Don't using namespace std;

Here is a great Stack Overflow answer about why you should not have this in your program.

Naming Conventions

Stick to one naming convention in your code. I see snake_case, camelCase and PascalCase in your code (and also Pascal_Snake_Case?). It makes your code look neater and a whole lot messier.

Guard Clauses

Instead of

if (...) {
    return;
} else {
    ...;
}

do this

if (...) {
    return;
}
...;

std::endl; vs "\n"

Consider using std::cout << "..." << std::endl; instead of ending a new line. While there is performance to think about with using std::endl, for a program like this, it's negligible. Here is another Stack Overflow link that talks about this.

Checking user input

Instead of instantly entering a while loop, get user input once after the prompt. Then, have the while loops only function be checking the input, displaying the error message, and getting new input. Once you have correct input, the while loop will exit and continue with the program. Look below:

std::string user_input;
std::cout << "\n***** Login or Register *****\n" << std::endl;
std::cout << "\n=============================" << std::endl;
std::cout << "\n***** Enter (login) or (register) *****\n" << std::endl;
std::getline(std::cin, user_input);

while (user_input != "login" and user_input != "register") {
    std::cout << "\n*Please write (login) or (register) correctly*\n" ;
    std::getline(std::cin, user_input);
}
if (user_input == "login"){
    login();
}
if (user_input == "register"){
    registry();
}

While there are other aspects you can improve on, I'm short on time. I'll edit this answer later, or let another reviewer provide their input.

\$\endgroup\$
3
\$\begingroup\$

In no particular order

  • your naming of bools is not very good. What does 'if_ON' represent. Or 'oo' or 'On_Off'. You need to give them names that have meaning. if_ON should be called 'alreadyRegistered' for example

  • you need a naming style, you have 'if_ON', 'Password', 'newName' all at the same scope. Pick a style and stick with it

  • move the file names, retry count etc to constants so that they can easily be changed

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.