Trouble with login multiple account

[merlinf]

I know I'm troublesome because I always ask about a topic but I'm really don't understand it. This time, it's about login account. So this function of mine work pretty well with the first succeed login, but if the first in a success, the second is always a success too, even if the pass or username is incorrect. I think it's about bool variables but not really sure.

  void login (int i)
{
	cout << "Enter username: ";
	cin >> username[i];
	ifstream file("accountdata.txt");
	if (!file.is_open())
        cout<<"could not open file\n"; 

    cout << "Enter Password: ";
    cin >> password[i];
    
    bool a = true;
    
    if (file)
    {
	    for (int n = 0; n <= 50; n++)	
	    {
	    	file >> line[i];	    		
			for (int j = 0; j <= 50; j++)	
				if (line[i][j] == username[i][j])
				{
					a = true;					
				}	
				else { a = false; break; }
							
			if (a)
			{
				memcpy(player[i].name, line[i], 50);
				break;
			}						
		}
						
		if (a)
		{			
			file >> line2[i];
			for (int j = 0; j <= 50; j++)	
				if (line2[i][j] == password[i][j])
				{
					a = true;					
				}	
				else { a = false; break; }
		}
		
		if (a)
		{			
			file >> player[i].victotal;
		}
		
		
			
		if (a)
			loginsuccess = 1;
	
						
		if (loginsuccess == 1)
		{
			file.close();
			cout << "You are being logged in! \n";
			cout << "What do you want to do?\n[1] Play game\n[2] Delete account\n";
			cin >> choice;
			switch(choice)
			{
				case 1: break;
				case 2: {account_delete(i); login_menu(i);}
			}		
		}	
		else
		{
			cout << "Invalid account \n";
			login_menu(i); 
		}
	}					
    
}

[jonnin]

loginsuccess variable is not defined?
what is the purpose of the 'a' bool?

most likely, assuming loginsuccess is a global (this is horrible!), it may just need to be set to false at the top of the function. If its global, it keeps its value from last time.

much better would be to make the function itself bool and return success/fail there.

bool login(whatever)
{
    blah blah
    ....
     if(username and password are correct)
        return true;
     return false;   
}

int main()
{
    etc 
    & so on;
    ...
    something = login(things);
    if(something)
     grant_access(to something);
}

its not wrong to check 'a' 20 times in a row, but its hard to read and terrible format.
just say
if(a)
{
all the crap that you do if a is true
}

[AbstractionAnon]

Lots of things wrong here.
- Lots and lots of globals which you didn't show, so we don't know if they're right. Globals should be avoided.
- L1: Not clear what i is for. Poor variable name, or at least include a comment explaining i.
- L6: If file doesn't open, you continue as if everything is okay.
- L12: What is a for? Poor variable name.
- Use of hard coded constant 50 in many places. Presumably this is the max number of players, but that's not clear. Should use a constexpr.
- L14: This whole if brackect is unnecessary if you return false] after line 7.
- Line 18: Why is line global? Looks like an array of char, not a std::string. Variables should be as local as possible.
- L20: Looks like you're comparing char by char. You can compare two std:strings directly.
- L28: You don't need to use memcpy if you're using using std:string.
- L55: If you call login multiple times, does loginsuccess get reset before each call? login should return a bool and not set a global.
L78: Why are you calling login_menu after saying it's an invalid account? Does login_menu call login again? This would be recursive. You don't want login to be recursive.

Where do you keep track of the number of unsuccessful login attempts for a user?

I'm somewhat confused. Are you writing in C or C++? This sure looks like C code with the exception of cin and cout.

[merlinf]

I'm so sorry if it's that unclear. My program is a lot longer. I found my program only wrong in this part so I think it would be easier for you to check T_T. I'm writing in c++ and my lecturer said I can only use char because I'm learning Introduction to Programming and it's better to understand the logic

[AbstractionAnon]

Okay, that makes things clearer.
Don't know if you're allowed to use strcmp.
This should help you:

#include <iostream>
#include <fstream>
using namespace std;

struct player_t
{
	char name[50];
	char password[50];
	int wins;
};
constexpr int MAX_PLAYERS{ 50 };

//
//	Globals
//
player_t players[MAX_PLAYERS];
int num_players{ 0 };
bool players_loaded{ false };

//	Format is:
//		line 1: Player name
//		Line 2:	Password
//		Line 3:	Wins
//	Returns true if file loaded successfully
bool load_players()
{
	ifstream file("accountdata.txt");
	player_t	temp;

	if (!file.is_open())
	{
		cout << "could not open accountdata.txt\n";
		return false;
	}
	while (file >> temp.name && num_players < MAX_PLAYERS)
	{	players_loaded = true;
		file >> temp.password;
		file >> temp.wins;
		players[num_players++] = temp;
	} 
	file.close();
	return true;
}

//	Attempt to login a user
//	User gets three attempts
//	Returns index to players array if found
//	Otherwise returns -1
int login ()
{
	char username[50];
	char password[50];
	int login_attempts{ 0 };
	constexpr int MAX_LOGIN_ATTEMPTS{ 3 };
	
	if (!players_loaded)
	{	//  Load the file ONCE
		if (!load_players())
			return -1;
	}
	cout << "Enter username: ";
	cin >> username;
	cout << "Enter Password: ";
	cin >> password;

	do
	{
		for (int i = 0; i < num_players; i++)
		{
			if (strcmp(players[i].name, username) != 0)
				continue;		// examine next entry
			if (strcmp(players[i].password, password) == 0)
				return i;		//  Found matching player			
		}
	} while (++login_attempts < MAX_LOGIN_ATTEMPTS);
	return -1;
}

[MikeyBoy]

I'm so sorry if it's that unclear. My program is a lot longer.

In which case, it would be helpful for you to compile a minimal, compilable codeset that demonstrates the problem.

This isn't just helpful to us, but to you, too; often, in stripping down the code to a more simple codeset, you'll discover for yourself what was causing the problem.

[seeplus]

L19-20. You're comparing 50 chars of both variables (are these c-style null-terminated?) - no matter how many of these chars are actually used. How are line and username defined? Are they null value initialised? If they are not, there's no expectation that the chars following the null terminator are going to be equal. If c-style strings, then use strcmp() to compare. If these are std::string, then you don't need the j loop and can just use == to compare.

Same for L36-37

L28 - use strcpy() and not memcpy()

The function should return bool - true if logged in or false otherwise. l55-70 should be in the caller function. There's no need to have loginsucess - as that is the return value.

What is L16 for? The maximum number of login attempts? If yes, then have 50 as a const with a name. The same for 50 used elsewhere (maximum size of array?). Make it a const with a name.

[seeplus]

Possibly something like:

#include <iostream>
#include <fstream>
#include <cstring>

constexpr auto NPOS{ static_cast<size_t>(-1) };
constexpr size_t MaxPlay{ 10 };
constexpr size_t MaxName{ 50 };
constexpr size_t MaxAttempts{ 3 };

struct Player {
	char name[MaxName]{};
	char password[MaxName]{};
	unsigned wins{};
};

Player players[MaxPlay]{};
size_t noPlayers{ };

bool load_players() {
	std::ifstream file("accountdata.txt");

	if (!file)
		return (std::cout << "Could not open accountdata.txt\n"), false;

	for (Player p; noPlayers < MaxPlay && (file >> p.name >> p.password >> p.wins); players[noPlayers++] = p);
	return true;
}

size_t login() {
	size_t logins{ };

	if (noPlayers == 0)
		if (!load_players())
			return NPOS;

	do {
		Player p{};

		std::cout << "Enter username: ";
		std::cin >> p.name;

		std::cout << "Enter Password: ";
		std::cin >> p.password;

		for (size_t i = 0; i < noPlayers; ++i)
			if (strcmp(players[i].name, p.name) == 0 && strcmp(players[i].password, p.password) == 0)
				return i;

	} while ((std::cout << "Invalid login\n") && (++logins < MaxAttempts));

	std::cout << "Maximum allowed login attempts reached\n";
	return NPOS;
}

@AbstractionAnon - shouldn't the username/password prompt/input be within the do loop - otherwise a wrong id/password will always be used for the max nukber of invalid logins?