Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is a console app I wrote for an end-of-chapter assignment in Jumping into C++. The parameters were for it to have a basic password system, and if you are eager, a multiple password / username system. I am going through this book completely on my own free will. It's not through a school assignment or whatnot.

It works quite well, but as I have a limited programming background, I wanted to post it here so any bad techniques or poor syntax could be brought to my attention.

#include <iostream>

using namespace std;

string usrnam[] = { "admin", "user", "" };
string pasword[] = { "root", "default", ""};

string temp_pass;
string temp_usr;

bool loggedin;

int n = sizeof(usrnam) / sizeof(string);

int findPassword( string str[], int array_size, string query )
{
    for ( int i = 0; i < array_size; i++ )
    {
        if ( str[i] == query )
    {
        return i;
    }
}
}

int commandC()
{
    int stage;
    cout << "Enter '1' for calculator: ";
    cin >> stage;
    switch(stage)
{
    case 1: {
        cout << endl;
        cout << "Not fully developed" << endl << endl;

    }
}

cout << "_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-";
}

void login(string user, string pass){
     if ( pass == pasword[findPassword(usrnam, n, user)] )
     {
        cout << "Logged in as " << user << endl << endl;
        loggedin = true;
        commandC();
     }
}

void registerUser()
{
    string answer;
    cout << "No matches found in the database...would you like to make new account? (Y/N)";
    cin >> answer;
    if (answer == "y" || answer == "Y")
    {
        cout << "Please enter a username: ";
        cin >> usrnam[n-1];
        cout << "Please enter a password: ";
        cin >> pasword[n-1];

        login(usrnam[n-1],pasword[n-1]);
    }
}

int main()
{
    cout << "Enter username: ";
    cin >> temp_usr;
    cout << "Enter password; ";
    cin >> temp_pass;

    login(temp_usr,temp_pass);

    if(!loggedin)
        registerUser();
}
share|improve this question
    
is this int n = sizeof(usrnam) / sizeof(string); division, or are you initializing two variables on the same line? –  Malachi Dec 12 '13 at 18:39
    
That is some code I found while looking around on the web to fix a problem I was having. It determines how many entries there are in the array "usrnam" –  Phaenomena Dec 12 '13 at 18:44
2  
Learn from Ken Thompson's regret: "If I had to redesign UNIX, I'd spell creat with an e." –  200_success Dec 12 '13 at 19:27
1  
@Malachi: It's a standard C idiom equivalent to int n = count(usrnam); –  Matt Dec 13 '13 at 0:33
    
You should never actually store the password. If your password DB falls into bad hands the bad people can then use the same passwords for other systems your users uses. So always hash your passwords before storing. That way even if your password DB is stolen nobody will know the actual password. –  Loki Astari Dec 14 '13 at 18:28

8 Answers 8

  • Try using std::vector instead of arrays. As a result, you'll not be required to keep track of the array_size in a separate variable like n in your case.

    Something like this:

    std::vector<std::string> username;
    username.push_back("admin");
    //other entries go here
    std::vector<std::string> password;
    password.push_back("root");
    //other entries go here
    

    Then, the size of the std::vector is given by size().

  • Try to use proper names as variables. Avoid wrong spellings or short-names. Exact variable names make the code easy to understand, as the name represents the actual concept the variable is referring to.

  • Also, in registerUser() the variable answer can just be a char. This will help in saving a little memory. Like:

    char answer;
    std::cin>>answer;
    if(answer=='y' || answer=='Y')
    {
        //code goes here
    }
    
  • In your code, the method commandC() is defined to return an int. But it doesn't do so. Make the return type void instead.

    void CommandC()
    {
        //code goes here
    }
    
  • Also, the main() method is defined to return an int too. It is a good practice to declare it like that, but make sure to return an integer. 0 (zero) is preferred as it refers to successful completion of main().

    int main()
    {
        //code goes here
        return 0; //indicates successful termination
    }
    
share|improve this answer
    
I was aware of the misspellings, but I am glad you brought it up. Thanks for the suggestions as well. –  Phaenomena Dec 12 '13 at 18:43
2  
@Phaenomena +1 :) Good to see your attitude to learn :) –  binaryBaBa Dec 12 '13 at 19:28
7  
This isn't Fortran. There's no reason to manage parallel arrays or vectors. Create a struct and then create a vector of that struct. –  jliv902 Dec 12 '13 at 19:55
1  
@jliv902 +1 Oh! I missed it. Thanks for reviewing the review. –  binaryBaBa Dec 13 '13 at 4:55
1  
@jiliv902: Or have a map that stores the passwords, indexed by user name. –  Christopher Creutzig Dec 14 '13 at 20:39

You have some compiler warnings:

$ clang++ -o console{,.cpp}
console.cpp:24:1: warning: control may reach end of non-void function [-Wreturn-type]
}
^
console.cpp:41:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
2 warnings generated.

You have a segfault:

Enter username: a
Enter password; b
Segmentation fault: 11

For entering a wrong password, I wouldn't expect to be invited to create an account:

Enter username: user
Enter password; WrongPassword
No matches found in the database...would you like to make new account? (Y/N)y
Please enter a username: admin
Please enter a password: LaxSecurity

If a password contains a space, weird things happen:

Enter username: user
Enter password; correct y horse battery staple
No matches found in the database...would you like to make new account? (Y/N)Please enter a username: Please enter a password: Logged in as horse

Enter '1' for calculator: _-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-
share|improve this answer
    
As for the first few problems, they may be fixed now as I've revised my code. The second "problem" isn't a problem at all, it's meant to be that way. And for the third problem, I'm currently working on a function that removes spaces from the input. –  Phaenomena Dec 12 '13 at 20:46
3  
Stripping spaces is the wrong approach. Use std::getline(std::cin, password). –  200_success Dec 12 '13 at 20:52
    
So do I need to change all my other "cin >>" statements now? I've read that mixing getline and cin is not the best thing to do. –  Phaenomena Dec 12 '13 at 20:58
    
Well, you are expecting lines of input, not words, so they should all use getline(). –  200_success Dec 12 '13 at 21:01
    
I replaced the username and password inputs to getline, but I left the others with cin. Good enough I guess. It still seems to accept the whitespaces though. –  Phaenomena Dec 12 '13 at 21:07
  • These are all being used as global variables:

    string usrnam[] = { "admin", "user", "" };
    string pasword[] = { "root", "default", ""};
    
    string temp_pass;
    string temp_usr;
    
    bool loggedin;
    
    int n = sizeof(usrnam) / sizeof(string);
    

    This is bad practice because any of these variables can be modified anywhere at any time, which is a source of bugs. Both of those initialized strings and n should be const, which will prevent them from being modified further. The rest of the variables should be declared in functions and possibly passed to other functions for use. This will allow you to keep track of how they're being used.

  • Passing in a C-style array isn't preferred:

    int findPassword( string str[], int array_size, string query ) {}
    

    This will actually deprecate to a pointer, which is not what the user expects to happen. This is also common practice in C, but you're writing in C++. As @gargankit mentioned, prefer to use an STL container such as std::vector in place of all the C-style arrays to avoid this deprecation. It'll also make it needless to pass in array_size since all STL containers know their sizes (accessible with the accessor function size()).

  • switch seems needless here:

    switch(stage)
    {
        case 1: {
            cout << endl;
            cout << "Not fully developed" << endl << endl;
        }
    }
    

    If you only have one case, just use an if:

    if (stage == 1)
    {
        cout << endl;
        cout << "Not fully developed" << endl << endl;
    }
    

    You could also make the body just one line, and with "\n" instead of std::endl. The latter also flushes the buffer, which takes longer. No need to use it for every newline.

    cout << "\nNot fully developed\n\n";
    
share|improve this answer
    
I didn't notice that I copied you. Imitation is the best flattery, no? :-D –  Fiddling Bits Dec 13 '13 at 1:21
    
@BitFiddlingCodeMonkey: No worries. :-) You didn't copy it entirely, plus you expanded on one of the variables quite effectively (minus the C-ness, haha). I merely pointed its wrongfulness and why it's wrong. –  Jamal Dec 13 '13 at 1:24
    
Thank you kindly. It is true what you say... every morning I wake up and say to myself, "Why did you waste your life with C!" ;-) –  Fiddling Bits Dec 13 '13 at 1:31
1  
@BitFiddlingCodeMonkey: C still has its uses, and C++ is just another language. Fortunately for you, you could easily focus on algorithms rather than having to utilize the STL (although I personally like the STL and I don't do too well with my own algorithms at times). –  Jamal Dec 13 '13 at 1:34

In the following:

int findPassword( string str[], int array_size, string query )
{
    for ( int i = 0; i < array_size; i++ )
    {
        if ( str[i] == query )
    {
        return i;
    }
}
}

If you use std::vector as suggested by gargankit, or you use std::array, you would be able to take advantage of the range-based for loops in C++. Instead of:

for(int i = 0; i < array_size; ++i)
{
    if(str[i] == query)
    {
       return i;
    }
}

you would write

for(const auto & arrayElement : str)
{
    if(arrayElement == query)
    {
        // note in this case arrayElement is a string, not
        // an integer index into the array, so this would
        // require changing the return type of findPassword
        return arrayElement;
    }
}

The benefit of a range-based for loop is that you don't have to be concerned about the potential risk of stepping out of the bounds of the given container, in this case str.

If you are feeling adventurous you might also look into using the standard algorithms included in the STL and calling either std::find_if, or std::any_of to find the array element you are looking for.

For functions that you are writing which take parameters that you will not modify in the body of the function, prefer taking the parameters as const parameters to specify and enforce your intent not to modify those values.

For example in:

void login(string user, string pass){
     if ( pass == pasword[findPassword(usrnam, n, user)] )
     {
        cout << "Logged in as " << user << endl << endl;
        loggedin = true;
        commandC();
     }
}

You never modify the value of pass, and it would be particularly strange in this case if you did modify the password that the user was typing in. In such a case it is more clear and more correct to require pass be const in login:

void login(string user, const string pass){
     if ( pass == pasword[findPassword(usrnam, n, user)] )
     {
        cout << "Logged in as " << user << endl << endl;
        loggedin = true;
        commandC();
     }
}

You can probably also do this to user, but that would require also making user const in the findPassword function.

Also, for any variables that you pass along as const, consider passing them as "const &" so that you do not make any unnecessary copies of that data.

share|improve this answer
2  
pass should actually be passed in as const&. If you're not modifying an object, doing this would avoid unnecessary copying. –  Jamal Dec 12 '13 at 19:18
    
I attempted to change the findPassword loop, but it appears I am in "C++98 mode". I'm using Codeblocks. Should it be C++98 or should I change it? –  Phaenomena Dec 12 '13 at 21:24
    
If you can change it to C++11 you will be better off. C++11 is more modern and will help you develop better practices and habits that are up to date with the latest standard for C++. Also, the vast majority of C++11 still allows C++98 functionality so I doubt you'll run into too much trouble performing the exercises in your book even if the book is expecting C++98 as the compiler. –  YoungJohn Dec 12 '13 at 21:31
    
@Phaenomena: Range-based for-loops are only available in C++11. In C++98, you can still use iterators by doing for (auto iter = container.begin(); iter != container.end(); ++iter) {} –  Jamal Dec 12 '13 at 21:31
    
@Jamal auto however is in C++11 not C++98. So the loop may look more like for(std::vector<int>::iterator iter = container.begin(); iter !- container.end(); ++iter) {} –  YoungJohn Dec 12 '13 at 21:32

You're using a lot of global variables, such as bool loggedin;. It's best to keep a variable's scope as small as possible. To make the scope smaller for loggedin, for example, remove this global declaration:

bool loggedin;

Do the following to the login function:

bool login(string user, string pass)
{
    bool loggedin; // Make a local copy of this in login

    if(pass == pasword[findPassword(usrnam, n, user)])
    {
        cout << "Logged in as " << user << endl << endl;
        loggedin = true;
        commandC();
    }
    else
    {
        loggedin = false; // Always make sure variables are initialized to something
    }

    return loggedin; // Return this value to calling function
}

Now you can call login like this:

int main()
{  
    cout << "Enter username: ";
    cin >> temp_usr;
    cout << "Enter password; ";
    cin >> temp_pass;

    bool loggedin = login(temp_usr,temp_pass); // Make another local copy of loggedin in main

    if(!loggedin)
        registerUser();
}
share|improve this answer
    
Regarding scope, you could just do bool loggedin = login(temp_usr,temp_pass); and remove the earlier declaration. :-) –  Jamal Dec 12 '13 at 21:42
    
@Jamal I'm paid by the line. ;-) Also, I have a heavy C background… so I'll blame that! –  Fiddling Bits Dec 12 '13 at 21:51
    
Forgive me. Us humans are yet to truly understand the way of the code monkey. :-) And I have a C++ background (barely any C), so there's that. –  Jamal Dec 12 '13 at 21:53
    
@Jamal It's the convention, in C, to put all a function's variables at the top. I bet you'd cringe looking at the stuff I've written! :-)) –  Fiddling Bits Dec 12 '13 at 21:56
    
Eh, I'd just be bothered if there were a "list" of variables at the top of a function. –  Jamal Dec 12 '13 at 21:58

In terms of best practice, you should keep in mind that passwords should never be stored raw, only as salted hashes. If you are going to ever write a program that will handle passwords in actual use (not just as a programming exercise), you really need to read Salted Password Hashing - Doing it Right. (Even if not, it's still a good read anyway.)

The reason for this is not because your application necessarily needs to be secure, but to ensure someone doesn't hack your application and steal your users accounts for other services, because most people use the same username and password everywhere.

Also, one other feature that would normally be needed is the ability to store the user data in a file and read it when it starts, so it can persist between sessions, although this too is probably beyond the scope of the excersise.

With respect to other aspects of your code, it seems you are on the right track; everything else I can see has already been said by other users.

share|improve this answer
    
+1 - In addition to the possibility of leaching username/passwords for other sites, this also falls into practice like your not practicing. Raw passwords (or passwords storage done wrong) should be avoided. –  WernerCD Dec 13 '13 at 14:04

you should look at the way you are indenting your code, this piece of code almost confused me for a minute ( or two )

int findPassword( string str[], int array_size, string query )
{
    for ( int i = 0; i < array_size; i++ )
    {
        if ( str[i] == query )
    {
        return i;
    }
}
}

it should look like this

int findPassword( string str[], int array_size, string query )
{
    for ( int i = 0; i < array_size; i++ )
    {
        if ( str[i] == query )
        {
            return i;
        }
    }
}

it is always good to keep your indentations straight (even thought you could write the code without them) it makes the code easier to read.

share|improve this answer
    
It's indented flawlessly in my IDE, but something got messed up while formatting it into my post. xD Thanks for bringing it up though. –  Phaenomena Dec 12 '13 at 18:40
    
what about that integer declaration? –  Malachi Dec 12 '13 at 18:41
    
Nevermind, I replied to your question –  Phaenomena Dec 12 '13 at 18:43

Usually comment about this line (look it up in every other C++ review).

using namespace std;

Don't use C arrays. The C++ containers are much more useful. Every other answer has suggested you use std::vector<std::string> as a replacement. That again is a terrible idea. Your main usage pattern is searching. Use a data structure that is designed to be searched. std::map<std::string, std::string>

string usrnam[] = { "admin", "", "" };
string pasword[] = { "root", "default", ""};

// I would have used:

std::map<std::string, std::string> password = {{"admin","root"},{"user","default"}};

I can see putting the actual username/password in global scope (as it needs to be used from everywhere (not a great design but acceptable). But no other variables should be in the global scope they should be close to the point of usage.

string temp_pass;
string temp_usr;

This should have been const. Your current design did not allow for modification of the username/password so this should have been const. But if you use a standard C++ container this becomes superfluous.

int n = sizeof(usrnam) / sizeof(string);

// Also note it is more traditional to write this as:
int n = sizeof(usrnam) / sizeof(usrnam[0]);

// This is because if you change the type of `usernam` this code is still
// correct. In your version you would need to change the code in two places.

C++ requires you to return a value at the end of a function:

int findPassword( string str[], int array_size, string query )
// This function does not return anything when the name is not
// found. This will result in hard to find errors.

When dealing with user input read a line at a time then parse the line.

cin >> stage;

// What happens if the user entered 1 12 <enter>
// The next time you are looking for input then you will read 12
// even though it is left over from this input.
// 
// Users interact with the terminal in lines.
// Because there input is not flushed until they hit `enter`
// they will hit enter after their input. The program does not recognize
// this and the operator>> does not remove the resulting '\n' character
// from the input. So if the next interaction with the user is getline()
// you will mysteriously read an empty line even if the user enters text.

Always use a break at the end of case. The exception is so rare that it should be well documented with comments.

Always put a default in a switch. Even if the default (throws an exception). An exception is better than the code blowing up because some other piece of code has changed.

switch(stage)
{
    case 1: {
       cout << endl;
        cout << "Not fully developed" << endl << endl;
    }
}

Password design.
Absolutely NEVER store the plain text version of the password. Always use a one way hash like md5 on the password and store the hash of the password.

Many people use the same password on multiple systems. If you lose control of your passwords then your have compromised your customers security.

So accept the password in plan text but always store it has a hash. You may also want to read up on slating the password first.

share|improve this answer
    
+1. Now it seems obvious that std::map is best for searching. My instinct still seems to be to almost always recommend std::vector in place of a C-style array (although my idea was a bit more general as I was mentioning why passing in a C-style array in C++ is not a good idea). –  Jamal Dec 14 '13 at 18:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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