Ways of making this program better,

Nov 1, 2011 at 10:21am
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!
Nov 1, 2011 at 10:46am
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.
Nov 1, 2011 at 11:26am
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 Nov 1, 2011 at 11:29am
Nov 1, 2011 at 12:06pm
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.
Nov 1, 2011 at 1:54pm
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 Nov 1, 2011 at 1:57pm
Nov 1, 2011 at 2:54pm
@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.
Nov 1, 2011 at 2:59pm
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...
Nov 1, 2011 at 5:48pm
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.
Nov 2, 2011 at 12:44pm
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.
Nov 2, 2011 at 11:20pm
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.
Nov 2, 2011 at 11:43pm
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 Nov 3, 2011 at 2:29pm
Nov 3, 2011 at 9:29pm
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 Nov 3, 2011 at 10:14pm
Topic archived. No new replies allowed.