Ways of making this program better,

So for the past fifteen minutes i have been making this code


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 #include <iostream>

using namespace std;

int main()
{
    string name;
    int pass;
    cout << "Input your name" << endl;
    cin >> name;
    cout << "Enter your password" << endl;
    cin >> pass;
    if(name=="Mason"){
    if(pass==1223){
    {cout << "Welcome Mason!" << endl;}
    }
    }
if(name=="Amber"){
if(pass==8523){
cout << "Welcome Amber!";
}
}

}


And i was looking for ideas on how to improve on it, so any ideas are welcome!
Start with better formatting (not joking, that is quite important). Then, remove excess {}s (you only really need one pair). Then replace consecutive ifs with a single if and &&. You could then combine the remaining two ifs with ||, since they do the same thing.
Now if it's extra features, improved flexibility or a more friendly interface you want to hear about, I could give some more suggestions.
Tips for a friendlier interface would be nice, ill update my code soon =D
of course ill add it to a GUI as soon as i learn them,

im still quite new to C++ haha
Last edited on
The most basic thing would be to add cout << "Error!" if the user name and password don't match. A better one would be "Error! Try again" and looping back to "Input your name". You could wrap your program in a while(true) loop, but then there is no way to quit. It would be good to add a break after "Welcome ..." so that you don't get "Error!" after that. Still, a user who forgot his password may too want to quit. For that you'll have to add additional input. Think what type of loop would then be most comfortable.
Personally, I would check for names and passwords individually:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
#include <iostream>

using namespace std;

int main(){
    login();
}

void login(){
    string name;
    int pass;
    cout << "Input your name:" << endl;
    cin >> name;
    if(name != "Mason" && name != "Amber"){
        cout << "Username not recognized!" << endl;
        login();
    }
    pass(name);
}

void pass(string name){
    int pass;
    cout << "Welcome, " << name << "!" << endl;
    cout << "Please enter your password: ";
    cin >> pass;
    switch(name){
        case "Mason":
            if(pass != 1223){
                cout << "That is not a valid password!" << endl;
                login();
            }
            break;
        case "Amber":
            if(pass != 8523){
                cout << "That is not a valid password!" << endl;
                login();
            }
            break;
        default:
            cout << "Error: Username lost. Please enter username again." << endl;
            login();
            break;
        }
    cout << "Login successful!" << endl;
}
Last edited on
@ciphermagi, that's a good idea. Note, though, that recursion should never be used where a loop can. Also, you can only switch basic types.
Nested if, then.

I wouldn't really do it this way, anyways, in practicality; I would save the usernames and passwords in a binary save file and name it something innocuously innocent, and then read them into a deque of objects, then use a function to iterate through the list and find the correct one; if nothing, then the username or password isn't found.

But I figured for the sake of the simplicity of the original program...
It's not normal to let the user/hacker know whether it's the username or password which is wrong. So while the code might check them separately, it shouldn't say which was wrong.
Maybe you could use a list for the user names or a map with the usernames and passwords, and then use iterators to walk the map looking for the name, and then check if the password is good or not.
Jessy, like I said above, I would actually save them in a list and page through them to find them (saving them in an external file).

The reason I wrote the code like I did is to keep it in the same context as the OP.
check this one...

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
string usrnm = { "Mason", "Amber" };
string passwd = { "1223", "8523" };

bool login() {
     string name;
     string pass;
     cout << "Enter your name : ";
     cin >> name;
     cout << "Enter your password : ";
     cin >> pass;
     for( int i =0 ; i < 2;i++ ) {
          if( usrnm[i] == name && passwd[i] == pass) {
               cout << "Welcome " << name<< endl;
               return true;
          }
     }
     return false;
}


I made it bool, just if u want that result, or you can make it void.
Last edited on
Well, assorted suggestions from coding standards I've worked to...

#1 declare variables as late as possible (at point of use)
#2 favor pre-increment
#3 use const where possible
#4 avoid abbrs, apart from well-known ones like "info".
#5 leave a space between operators
#6 use structs to group related variables (so you don't accidentally add a username (e.g. in alphabetical order) and accidentally offset all passwords...)
#7 minimize the use of globals, and make them stand out (e.g. g_ prefix) when you have to use them
#8 use spacer lines to make steps clear (a. get user name, b. get password, c. check values)
#9 use an unsigned type (size_t is typically unsigned int) to store an integer that should never be negative

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
<iostream>
<string>
<limits>
<cstdlib>
using namespace std;

// for some reason, I tend to skip the g_ for notFound...
static const size_t notFound = numeric_limits<size_t>::max();

struct UserInfo {
    const char* username;
    const char* password;
};

static const UserInfo g_aUserInfo[] = {
  {"Mason", "1223"},
  {"Amber", "8523"}
};

static const size_t g_userInfoCount = _countof(g_aUserInfo);

...

size_t login() {
     string username;
     cout << "Enter your user name : ";
     cin >> username;
     cout << endl;

     string password;
     cout << "Enter your password : ";
     cin >> password;
     cout << endl;

     for( size_t index = 0; g_userInfoCount > index; ++index ) {
        if( g_aUserInfo[index].username == username &&
            g_aUserInfo[index].password == password       ) {
                cout << "Welcome " << username << endl;
                return index;
          }
     }

     cerr << "Login failed : invalid user name or password." << endl; 
     return notFound;
}
Last edited on
Topic archived. No new replies allowed.