8
\$\begingroup\$

I'm currently self-teaching myself C++, using a C++ For Dummies book that I bought a couple months ago. To practice, I've created a Bank class that holds usernames and passwords, and that has a login/logout system. IT also implements a failsafe if there are too many failed login attempts. I'm looking for advice that can help make this code better, i.e more efficient, more compact, and more up-to-date with todays C++ standards. Any and all help is appreciated and considered.

bank.cpp

//Include Statements
#include <iostream>
#include <vector>
#include <string>

//Class Body
class Bank {

    std::vector<std::string> usernames;
    std::vector<std::string> passwords;
    std::string user;
    int attempts;
    bool lockedOut;

    public:
        bool login(std::string username, std::string password);
        void logout();
        std::string getCurrentUser();
        //Constructor
        Bank() {
            generateTestAccounts();
            attempts = 0;
            lockedOut = false;
        }

    private:
        void generateTestAccounts();

};

//Class Functions

/*
* Checks if a user is not logged in, and if not, checks to see if
* the passed username and password match any in the Bank
*/
bool Bank::login(std::string username, std::string password) {
    if(!lockedOut) {
        if(user.empty()) {
            for(int i = 0; i < usernames.size(); i++) {
                if(usernames[i].compare(username) == 0) {
                    user = username;
                    return true;
                }
            }
        }
        attempts += 1;
        if(attempts == 3) {
            lockedOut = true;
        }
    }
    return false;   
}


/*
* Logs out the current user by clearing the string value from `user`
*/
void Bank::logout() {
    user.clear(); //basically null
}

/*
* Returns the currently logged in user
*/
std::string Bank::getCurrentUser() {
    return user;
};

/*
* Generates test accounts that can be used to make sure the
* `login` method works
*/
void Bank::generateTestAccounts() {
    for(int i = 0; i < 5; i++) {
        usernames.push_back("user" + std::to_string(i));
        passwords.push_back("pass" + std::to_string(i));
    }
}

/*
* main method for testing bank class
*/
int main() {
    Bank bank;
    std::string us = "user1";
    std::string pw = "pass1";
    if(bank.login(us, pw)) {
        std::cout << "Logged in as " + us << std::endl;
    }
}
\$\endgroup\$
6
\$\begingroup\$
  • You don't check for the password in the login member function. This is the least secure user credential system I've ever seen :)

  • You have some unnecessarily verbose comments in your code, e.g.

    //Include Statements
    //Class Body
    //Constructor
    //Class Functions
    user.clear(); //basically null
    

    Just remove them. Comments are hard to maintain and should be used with care, i.e., only when a piece of code is too hard to understand from the code structure and naming of variables, types and functions.

  • You define the constructor Bank::Bank() inside the class definition, but all other member functions outside of the class. Not that this is a severe issue, but I don't think there is a reason for treating the special member function differently with this regard, so consider unifying this.

  • Use in-class member initializers, this is an approach that works consistently across constructor overloads and is blessed by the core guidelines C.48.

    class Bank {
        int attempts = 0;
        bool lockedOut = false;
    
        // ...
    };
    

    Definitely don't assign them in the constructor body, see also Item 4 in Scott Meyer's Effective C++ ("Make sure that objects are initialized before they're used").

  • Try to avoid the use of getter member functions like getCurrentUser() if possible. If you do need one, prefer const-qualified member functions that return a const reference or something similar to avoid unnecessary copies. If C++17 is available, e.g.

    std::string_view getCurrentUser() const;
    
  • The state that no user is currently logged in is represented by an empty user name, you hence rely on some null-like state of a type. This is possible, but consider replacing it with something more expressive, e.g.

    bool hasCurrentUser() const;
    

    or (again, with C++17)

    std::optional<std::string_view> getCurrentUser() const;
    
  • You also have unnecessary copies here:

    bool login(std::string username, std::string password);
    

    Pass const std::string& or std::string_view instead.

  • Prefer returning early from a function if you need to check a precondition at the top of it:

    bool Bank::login(std::string username, std::string password) {
        if(lockedOut)
           return;
    
        // The rest...
    }
    

    This is more readable and requires less indentation.

  • Consider replacing attempts += 1; by ++attemps;. This is clearly not a severe issue, but it's a good attitude to minimize the number of magic numbers in your code (1 isn't that magic, but still...).

  • This definitely comes closer to a magic number:

    if(attempts == 3)
    

    Prefer adding a static constexpr maxLoginAttempts = 3; to your class. In addition, if (attempts > maxLoginAttempts) seems a bit more robust to me if you change some implementation details afterwards.

  • Avoid too many level of nested branches, in my opinion, this is already too much:

    if(!lockedOut) {
        if(user.empty()) {
            for(int i = 0; i < usernames.size(); i++)
               // ...
    
            // ...
        }
    
        // ...
    }
    

    You can easily fix this by moving parts of a member function into a new member function.

  • Use standard <algorithm>s when appropriate. Looking for a std::string in a std::vector<std::string> is a prime example for when it's appropriate:

    if (std::find(usernames.cbegin(), usernames.cend(), username) != usernames.cend()) {
       user = username;
       return true;
    }
    
  • Only one user is able to login to your bank at a time, and the only way to add new users that are able to login is to modify the internals of the Bank class. Also, passwords are stored in plain text, and if the number of users grow, a std::vector might not be the best choice for looking up hash-able user identification tokens (like the username). I am aware that this is an exercise, just as a hint where you could go from here.

\$\endgroup\$
  • \$\begingroup\$ Whoops! Totally forgot to add the password part haha! Thanks for the excellent and in-depth answer! \$\endgroup\$ – Linny Apr 4 '19 at 13:37
3
\$\begingroup\$

This is a quick review to cover an important issue not already mentioned.

Fix the bug

When a user logs out, shouldn’t attempts be reset to 0? Otherwise a single user can effectively lock out the entire bank. There are other circumstances that should also reset attempts so you should think carefully about where those are.

Rethink the class operation

Does it really make sense to lock the entire bank in response to excessive failed login attempts? Or would it make more sense to lock the single account? Also, the username and password are closely associated but in two separate data structures. I’d be inclined to define an Account class and have a vector of those.

\$\endgroup\$
1
\$\begingroup\$

Use a std::map to store user name and password

Use of

std::vector<std::string> usernames;
std::vector<std::string> passwords;

for storing user names and corresponding passwords is not good. It's easy to add the same user name multiple times. I think it will be bette to use a map.

// Key is user
// Value is password
std::map<std::string, std::string> users;

Add a function to add a user

Add a function to add a user. In the function, make sure the user name is not already used before adding it to the map.

bool addUser(std::string const& name, std::string const& pass)
{
   if ( users.find(name) != user.end() )
   {
      std::cerr << "User '" << name << "' already exists.\n";
      return false;
   }

   user[name] = pass;
   return true;
}

Update generateTestAccounts

Update generateTestAccounts to use addUser.

void Bank::generateTestAccounts() {
   for(int i = 0; i < 5; i++) {
      addUser("user" + std::to_string(i), "pass" + std::to_string(i));
   }
}

Update login to use the map

bool Bank::login(std::string const& username, std::string const& password) {
    if(lockedOut)
    {
       return false;
    }

    if ( users.find(username) == users.end() )
    {
       std::cerr << "Unknown user: '" << username << "'\n";
       return false;
    }

    if ( users[username] == password )
    {
       user = username;
       return true;
    }

    attempts += 1;
    if(attempts == 3)
    {
       lockedOut = true;
    }

    return false;   
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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