patterncppMinor
Fake bank account class
Viewed 0 times
bankclassaccountfake
Problem
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 usernames;
static vector passwords;
string firstName, lastName;
string username, password;
};
vector bankAcc::usernames;
vector bankAcc::passwords;
bool bankAcc::createUser(istream &is) {
is >> username >> password;
if (is.fail()) {
cout > 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;
}Solution
I have a few suggestions:
In your constructor, you are passing in
Also, this constructor should check whether or not
Speaking of
You should also practice the
Finally, your use of two vectors to store the user-name and password is inefficient since you end up spending
Here's your code refactored a bit:
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 uname_to_pass;
string firstName, lastName;
string username, password;
};Code Snippets
bankAcc(const string &fn, const string &ln) { /*...*/ }bankAcc(const string &fn, const string &ln)
{
if (fn == "" || ln == "")
throw std::invalid_argument("Can't have empty names");
firstName = fn;
lastName = ln;
}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;
};Context
StackExchange Code Review Q#114791, answer score: 2
Revisions (0)
No revisions yet.