Fake bank account class

Question

I just some reviews on how I can make it more efficient or cleaner and what I should fix in general.

class bankAcc
{
public:
    bankAcc() = default;
    bankAcc(string fn, string ln) : firstName(fn), lastName(ln) {}
    bool createUser(istream &is);
    bool login() const;
private:
    bool correctLogin(const string&, const string&) const;
    static vector<string> usernames;
    static vector<string> passwords;
    string firstName, lastName;
    string username, password;
};

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

bool bankAcc::createUser(istream &is) {
    is >> username >> password;
    if (is.fail()) {
        cout << "Invalid option" << endl;
        cin.clear();
        return false;
    }
    usernames.push_back(username);
    passwords.push_back(password);
    return true;
}

bool bankAcc::login() const {
    bool login;
    string tempUser, tempPass; 
    cin >> tempUser >> tempPass;
    if (cin.fail()) {
        cout << "Invalid options" << endl;
        cin.clear();
        return false;
    }
    login = correctLogin(tempUser, tempPass);
    if (login == true) {
        cout << "Logged on" << endl;
        return login;
    }   
    else {
        cout << "Incorrect info" << endl;
        return login;
    }
    return login;
    }

bool bankAcc::correctLogin(const string &tempUser, const string &tempPass) const {

    if (usernames.empty() || passwords.empty())
        return false;
    bool usernamePassed, passwordPassed; int cnt = 0;
    for (auto c : usernames) {
        if (tempUser == c) {
            usernamePassed = true;
            break;
        }
        else {
            usernamePassed = false;
        }
        ++cnt;
    }
    if (passwords[cnt] == tempPass && cnt < passwords.size()) {
        passwordPassed = true;
    }
    else {
        passwordPassed = false;
    }
    return usernamePassed && passwordPassed;
}

Answer 1

I have a few suggestions:

In your constructor, you are passing in string by copy when it is completely unnecessary to do so. You should pass these in by const reference:

bankAcc(const string &fn, const string &ln) { /*...*/ }

Also, this constructor should check whether or not fn or ln are clean inputs (i.e. they're valid first and last names). You can do this like so:

bankAcc(const string &fn, const string &ln) 
{
    if (fn == "" || ln == "")
        throw std::invalid_argument("Can't have empty names");
    firstName = fn;
    lastName = ln;
}

Speaking of firstName and lastName, I don't see a use for them here at all as they're only used in the constructor and nowhere else. Is there a need for them?

You should also practice the Principle of Sole Responsibility here. In other words, each class should have only one single responsibility to take care of. Your class is called bankAcc yet it really seems more like a BankAccountLoginValidator class to me. Your class isn't doing anything that a bank account should be doing (no balance, no withdrawal/deposit schemes etc). Moreover, you should generally separate I/O operations from the validation code in this class. Have the caller of the class objects perform the I/O and then have them use your class to validate their input. Your class should not be responsible for I/O.

Finally, your use of two vectors to store the user-name and password is inefficient since you end up spending O(n) time searching for the right username in the usernames vector. Instead, look at using a hashtable to store this information instead. A hash-table in C++11 is called std::unordered_map. Then, your lookup operation can be done in O(1) amortized time. With this map, your correct_login() function becomes unnecessary.

Here's your code refactored a bit:

class bankAcc
{
public:
    bankAcc() = default;
    bankAcc(const string &fn, const string &ln)
    {
        if (fn == "" || ln == "")
            throw std::invalid_argument("Error! Can't have empty names");
        firstName = fn;
        lastName = ln;
    }
    bool createUser(const string &user_name, const string &pass_word)
    {
        if (user_name == "" || pass_word == "")
            return false;
        /* Maybe add more validation code here */
        // ....

        uname_to_pass.insert(user_name, pass_word);
    }
    bool login(const string & user_name, const string &pass) const
    {
        return uname_to_pass[user_name] == pass;
    }

private:
    static unordered_map<string, string> uname_to_pass;
    string firstName, lastName;
    string username, password;
};