1
\$\begingroup\$

I was wondering if you'd be able to review my basic login system that uses sstream and fstream? I feel a lot is wrong with it in terms of efficiency. I've included validation such as if a username already exists they won't be able to use it.

To test it works, the text file needs the following format:

George,Password,

AwesomeUser321,Password123,

Jill,Password123,

Sam,321Password,

I'm hoping the code will be fairly logical. I have commented it for further information.

// FileHandling.cpp : This file contains the 'main' function. Program execution begins and ends there.

#include <iostream>
#include <sstream>
#include <fstream>
#include <string>
#include <vector>
using namespace std;

struct User{
    string sUsername;
    string sPassword;
};
bool Check_Text_File(vector<User>&objUsers, string &sUsername, string &sPassword)
{
    for(int iCount = 0; iCount < objUsers.size(); iCount++)
    {
        if (sUsername == objUsers.at(iCount).sUsername)
        {
            cout << "Sorry, someone already has this username.\n";
            cout << "Try again!\n";
            return true;
        }
    }
    return false;
}
bool Is_Login_Details_Correct(vector<User>&objUsers, string &sUsername, string &sPassword)
{
    for(int iCount = 0; iCount < objUsers.size(); iCount++)
    {
        if (sUsername == objUsers.at(iCount).sUsername && sPassword == objUsers.at(iCount).sPassword)
        {
            cout << "You have successfully logged in.\n";
            return true;
        }
    }
    cout << "Incorrect.\n";
    return false;
}
void Create_Account(vector<User>&objUsers, string &sUsername, string &sPassword)
{
    cout << "Enter a new username: ";
    getline(cin >> ws, sUsername);
    cout << "Enter your new password: ";
    getline(cin >> ws, sPassword);

}

void Append_Credential_To_Text_File(string &sUsername, string &sPassword)
{
    ofstream file;
    file.open("/Applications/Practice.cpp/Practice.cpp/Practice/doodle/HowQuickly/HowQuickly/new.txt", fstream::app);
    file << sUsername << "," << sPassword << ",\n";
    file.close();
}
void Last_Line_To_Vector(vector<User>&objUsers)
{
    const std::string filename = "/Applications/Practice.cpp/Practice.cpp/Practice/doodle/HowQuickly/HowQuickly/new.txt";
    std::ifstream fs;
    fs.open(filename.c_str(), std::fstream::in);
    if(fs.is_open())
    {
        //Got to the last character before EOF
        fs.seekg(-1, std::ios_base::end);
        if(fs.peek() == '\n')
        {
            //Start searching for \n occurrences
            fs.seekg(-1, std::ios_base::cur);
            long int i = fs.tellg();
            for(i;i > 0; i--)
            {
                if(fs.peek() == '\n')
                {
                    //Found
                    fs.get();
                    break;
                }
                //Move one character back
                fs.seekg(i, std::ios_base::beg);
            }
        }

        std::string lastline;
        getline(fs, lastline);
        std::cout << lastline << std::endl;
        string sResult = "";
        stringstream ssSplit(lastline);
        vector<string>objSplit;

        objUsers.push_back(User());
        objSplit.resize(0);
        while (getline(ssSplit, sResult, ','))
        {
            objSplit.push_back(sResult);
        }
        objUsers.at(objUsers.size() - 1).sUsername = objSplit.at(0);
        objUsers.at(objUsers.size() - 1).sPassword = objSplit.at(1);
    }
}



void Add_Text_File_To_Vector(vector<User>&objUsers, vector<string>&objSplit){
    string sLine = "";
    string sResult = "";
    ifstream file3("/Applications/Practice.cpp/Practice.cpp/Practice/doodle/HowQuickly/HowQuickly/new.txt");

    if(objUsers.size() == 0)
    {
        while (getline(file3, sLine))
        {
            stringstream ssSplit(sLine);
            objUsers.push_back(User());
            objSplit.resize(0);
            while (getline(ssSplit, sResult, ','))
            {
                objSplit.push_back(sResult);
            }
            objUsers.at(objUsers.size() - 1).sUsername = objSplit.at(0);
            objUsers.at(objUsers.size() - 1).sPassword = objSplit.at(1);
        }
    }
}
void Login_To_Account(vector<User>&objUsers)
{
    string sUsername;
    string sPassword;
    cout << "Enter your username: ";
    cin >> sUsername;

    cout << "Enter your password: ";
    cin >> sPassword;


}
int main()
{
    string sUsername;
    string sPassword;
    vector<User>objUsers;
    vector<string>objSplit;
//Add_Text_File_To_Vector() function reads everything from the file to the struct object objUsers before the user can subsequntly append a new detail which will be formatted in accordance to the format presented in the text file. 
    Add_Text_File_To_Vector(objUsers, objSplit);
    do
    {
        Create_Account(objUsers, sUsername,sPassword);
    }
    while(Check_Text_File(objUsers, sUsername, sPassword));
    //The Append_Crendential_To_Text_File() function appends the new login details the user types in the function "Create_Account()".

    Append_Credential_To_Text_File(sUsername, sPassword);
//The Last_Line_To_Vector appends the newly added credential to the vector, instead of re-writing the entire vector. 

    Last_Line_To_Vector(objUsers);

    do
    {
        Login_To_Account(objUsers);

    }while(Is_Login_Details_Correct(objUsers, sUsername, sPassword) == false);
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I think this code is very basic. I didn't look very deeply inside it and didn't bother to run it. I think I can give you some advices without it. I only looked at readability not efficiency, which when you are using standard c++ should be fine in most cases. You should usually be woried about efficiency only when you detect there is problem with it and it matters. There were some highlights in your code but there are things you should work at.

  1. hungarian notation. I don't want to get into this discussion, but I think nowadays it's not popular to use it. So imho: sUsername should be called username. sPassword - password objUsers - users etc...

  2. Lines in comments shouldn't be longer than code itself. It's very annoying to reader.

  3. In Add_Text_File_To_Vector:

file3 should be passed as argument (type of argument should be const std::istream &) and why is it called file3? Where is file1, file2...

Name of file should not be inside function. Functions should be generic, works in different environments. This is gonna work only in your filesystem with your directory structure.

You could define file in main in first line, eg:

std::string inputFilePath = "/Applications/Practice.cpp/Practice.cpp/Practice/doodle/HowQuickly/HowQuickly/new.txt"

Or better yet, program filename could be argument to program. I mean you could change int main() to int main(int argc, char** argv) and pass it as argv. This way you could try different input files without recompiling program.

Potential reviewer have to dive deep into your code to even run it.

I don't understand the point of this:

if (objUsers.size() == 0) {
    // loop code
}

You should just write code without this if condition. Without it program is gonna work the same way and function will be simpler and more generic. If you realy wanted to kind of assert, that input is correct you could write: assert(objUsers.size() == 0) This way program would immediately stopped on error - and this is good behavior for most programs.

It was very hard to figure out what objSplit is for. But I think you wanted to do some optimization because I don't see it is used anywhere. I think you should create this vector inside function. You shouldn't bother reader of your high level code with this low level detail. If it is realy true (it's not obvious), that resize(0) is faster than creating objSplit from scratch every time and it is realy gonna make a difference than you could make it a global variable. Premature optimization are usualy harmfull and this was very harmfull for readability. And it was not said in comment. I had to figure it out by myself. Comment near the function only stated obvious thing that I figured out before I read it.

I think this singature of function would be better:

 Read_Users_List(const istream & input, vector<User>& users) {}
  1. In Create_Account

you passed objUsers but function don't use it. This is plain wrong and hurts readability.

5 Check_Text_File

name of function is a lie. It doesn't check any text file inside it. It should be called for example Is_Username_Unique. And it should not has sPassword as argument, because it is not used anywhere.

If you use c++11 you could exchange:

for(int iCount = 0; iCount < objUsers.size(); iCount++)

with

for (const auto & user : objUsers)

and than use:

if (sUsername == user.sUsername)

instead of:

if (sUsername == objUsers.at(iCount).sUsername)

It is important to use this whenever you can. Because it is easier to read and more errorproof.

  1. Append_Credential_To_Text_File

You hardcoded name of the file with credentials. You write the same name of the file twice in your code. This is the worst thing you done so far. You should pass the name of file as argument or at least make it a global variable. In this program you have to change one thing in two places. If i wanted to run this, I'd have to change the path in two places.

  1. Last_Line_To_Vector:

the same filename again... This has so much duplication in it... and it is sooo complex. Couldn't you just write

objUsers.push_back({sUsername, sPassword}); 

instead:

Last_Line_To_Vector(objUsers);

Or at least you could write:

User objUser;
objUser.sUsername = sUsername;
objUser.sPassword = sPassword;
objUsers.push_back(objUser);

If I understood correctly that this is what you wanted to do...

  1. Login_To_Account

sUsername and sPassword does nothing. Shouldn't you return it somehow from function, for example add arguments string & sUsername, string & sPassword than Is_Login_Details_Correct would have more sense... You should pass to this function the same credentials.

while(Is_Login_Details_Correct(objUsers, sUsername, sPassword) == false);

should be just:

while(not Is_Login_Details_Correct(objUsers, sUsername, sPassword));

There are more things that could be done better, but let's start with this.

\$\endgroup\$
1
  • \$\begingroup\$ I am looking back at old questions and I am so sorry I never thanked you for your answer. I found this very beneficial! \$\endgroup\$ Nov 14, 2021 at 14:59

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.