ATM Machine Program for a Project

Question

I'm a novice C++ coder, taking my first class this semester, and we have a project coming up. I get the program done early and it works as expected, but I don't know if it's too messy and can be cleaned up. It's a simple ATM program that allows you to create multiple accounts and withdraw or deposit money in them. Any tips on how I can clean this up would be greatly appreciated!

Code:

#include <iostream> #include <iomanip> #include <string> #include <vector> using namespace std; class AutoTellerMachine { //Object to represent each customer who uses the ATM program public: void CreateNewAccount(string newUsername, string newPassword); void AccountLogin(string loginUsername, string loginPassword); void DepositMoney(double depositAmount); void WithdrawMoney(double withdrawalAmount); void SetAccountLogin(int setAccountLocation); void SetLastMoneyMovement(int accountID, double amount); void SetBeginningBalance(int accountID); void SetLastOperation(int accountID, char userInput); void AccountMenu(); int GetAccountLogin() const; double GetLastMoneyMovement(int accountID) const; double GetAccountBalance(int accountID) const; double GetBeginningBalance(int accountID) const; char GetLastOperation(int accountID) const; string GetUsername(int accountID) const; private: int loggedInAccountLocation; double accountBalance; double beginningBalance; double lastMoneyMovement; char lastOperation; string username; string password; }; AutoTellerMachine account; vector<AutoTellerMachine> AccountList; //This vector allows for multiple accounts to be stored so that if more than one person uses the account, all information is retained void AccountMenu(); void UserMenu(); void AutoTellerMachine:: SetAccountLogin(int setAccountLocation) { loggedInAccountLocation = setAccountLocation; return; } int AutoTellerMachine::GetAccountLogin() const { return loggedInAccountLocation; } void AutoTellerMachine::CreateNewAccount(string newUsername, string newPassword) { //Adds the new information to the vector to give the account personalized info int accountListSize = AccountList.size(); AccountList.at(accountListSize - 1).accountBalance = 0.0; AccountList.at(accountListSize - 1).username = newUsername; AccountList.at(accountListSize - 1).password = newPassword; } void AutoTellerMachine::AccountLogin(string loginUsername, string loginPassword) { int accountListSize = AccountList.size(); bool successfulLogin = false; int accountLocation = 0; for(int i = 0; i < accountListSize; i++) { if(loginUsername == AccountList.at(i).username) { if(loginPassword == AccountList.at(i).password) { successfulLogin = true; accountLocation = i; } } } if(successfulLogin != true) { cout << endl << "******** LOGIN FAILED! ********" << endl << endl; UserMenu(); } else if(successfulLogin == true) { SetAccountLogin(accountLocation); cout << endl << "Access Granted - " << AccountList.at(loggedInAccountLocation).username << endl; AccountMenu(); } return; } void AutoTellerMachine::DepositMoney(double depositAmount) { AccountList.at(loggedInAccountLocation).accountBalance += depositAmount; return; } void AutoTellerMachine::WithdrawMoney(double withdrawalAmount) { AccountList.at(loggedInAccountLocation).accountBalance -= withdrawalAmount; return; } double AutoTellerMachine::GetAccountBalance(int accountID) const { return AccountList.at(loggedInAccountLocation).accountBalance; } void AutoTellerMachine::SetLastMoneyMovement(int accountID, double amount) { AccountList.at(accountID).lastMoneyMovement = amount; } void AutoTellerMachine::SetBeginningBalance(int accountID) { AccountList.at(accountID).beginningBalance = AccountList.at(loggedInAccountLocation).accountBalance; } void AutoTellerMachine::SetLastOperation(int accountID, char userInput) { AccountList.at(accountID).lastOperation = userInput; } double AutoTellerMachine::GetLastMoneyMovement(int accountID) const { return AccountList.at(accountID).lastMoneyMovement; } double AutoTellerMachine::GetBeginningBalance(int accountID) const { return AccountList.at(accountID).beginningBalance; } char AutoTellerMachine::GetLastOperation(int accountID) const { return AccountList.at(accountID).lastOperation; } string AutoTellerMachine::GetUsername(int accountID) const { return AccountList.at(GetAccountLogin()).username; } void UserMenu() { //Implements a user interface that allows the user to make selections based on what they want to do char userSelection; string createUserId; string createUserPass; string usernameCheck; string passwordCheck; cout << "l -> Login" << endl; cout << "c -> Create New Account" << endl; cout << "q -> Quit" << endl << endl << ">"; cin >> userSelection; if((userSelection == 'l') || (userSelection == 'L')) { //Checks to make sure the login is valid and if not, couts an error statement cout << endl << "Please enter your user name: " << endl; cin >> usernameCheck; cout << "Please enter your password: " << endl; cin >> passwordCheck; account.AccountLogin(usernameCheck, passwordCheck); } else if((userSelection == 'c') || (userSelection == 'C')) { //Captures info for a new account cout << endl << "Please enter your user name: " << endl; cin >> createUserId; cout << "Please enter your password: " << endl; cin >> createUserPass; AccountList.push_back(account); //This creates a new object in the vector to be filled with the information gathered account.CreateNewAccount(createUserId, createUserPass); cout << endl << "Thank You! Your account has been created!" << endl << endl; UserMenu(); } else if((userSelection == 'q') || (userSelection == 'Q')) { //Exits the entire program cout << endl << "You selected quit!" << endl << endl; } else { cout << endl << "Invalid selection." << endl; UserMenu(); } return; } void AutoTellerMachine::AccountMenu() { //This is a separate menu from the user menu because it deals with all options available to a logged in customer char userInput; double amountOfDeposit; double amountOfWithdrawal; cout << endl << "d -> Deposit Money" << endl; //This has a couple more options than indicated in our project overview, but I feel they make this a more useable program cout << "w -> Withdraw Money" << endl; cout << "r -> Request Balance" << endl; cout << "z -> Logout" << endl; cout << "q -> Quit" << endl; cout << endl << ">"; cin >> userInput; if((userInput == 'd') || (userInput == 'D')) { //Deposit function that changes the balance of the account user and sets the last money movement for later use SetBeginningBalance(GetAccountLogin()); cout << endl << "Amount of deposit: " << endl; cin >> amountOfDeposit; SetLastMoneyMovement(GetAccountLogin(), amountOfDeposit); SetLastOperation(GetAccountLogin(), userInput); DepositMoney(amountOfDeposit); AccountMenu(); } else if((userInput == 'w') || (userInput == 'W')) { //Withdraw function makes sure that enough funds are present for the operation before removing money cout << endl << "Amount of withdrawal: " << endl; cin >> amountOfWithdrawal; if(amountOfWithdrawal > GetAccountBalance(GetAccountLogin())) { cout << endl << "******Insfficient Funds!*******" << endl; } else { SetBeginningBalance(GetAccountLogin()); SetLastMoneyMovement(GetAccountLogin(), amountOfWithdrawal); SetLastOperation(GetAccountLogin(), userInput); WithdrawMoney(amountOfWithdrawal); } AccountMenu(); } else if((userInput == 'r') || (userInput == 'R')) { //Simply prints the balance before the last transaction, what type and amount the last transaction was then the current balance cout << endl << "Beginning balance: $" << fixed << setprecision(2) << GetBeginningBalance(GetAccountLogin()) << endl; if(GetLastOperation(GetAccountLogin()) == 'd') { cout << "Deposit amount: $" << fixed << setprecision(2) << GetLastMoneyMovement(GetAccountLogin()) << endl; } else if(GetLastOperation(GetAccountLogin()) == 'w') { cout << "Withdrawal amount: $" << fixed << setprecision(2) << GetLastMoneyMovement(GetAccountLogin()) << endl; } cout << "Your balance is $" << fixed << setprecision(2) << GetAccountBalance(GetAccountLogin()) << endl; AccountMenu(); } else if((userInput == 'z') || (userInput == 'Z')) { //Allows the user to logout of their account and brings them back to the user menu so they can log in with a different account cout << endl << "You have successfully logged out, " << GetUsername(GetAccountLogin()) << "!" << endl << endl; UserMenu(); } else if((userInput == 'q') || (userInput == 'Q')) { //Exits the entire program cout << endl << "Thanks for banking with COP2513.F16, " << GetUsername(GetAccountLogin()) << "!" << endl; } else { cout << endl << "Invalid selection." << endl; AccountMenu(); } return; } int main() { cout << "Welcome to COP2513.F16’s ATM Machine" << endl << endl; cout << "Please select an option from the menu below: " << endl << endl; UserMenu(); }` 

Answer 1

So for an early project this already looks quite good, however there is still a lot to improve.

1. The obligatory "Do not use namespace std;"It is bad practice and will hurt you in the long run. So start typing std:: when you need it.

2. Whenever you pass data that should not be modified, pass it as const, so that you actually cannot modify it. Also you are passing the data via a copy here, a reference would make more sense. So for example your function CreateNewAccount should look like this

   void CreateNewAccount(const std::string &newUsername, const std::string &newPassword); 

   The same applies for functions that do not change the state of the class. mark them const.

3. Your naming is often confusing. AcountLocation seems to stand for AcountID. Please go through your variables check whether the name means the right thing.

4. The last element of a contain can usually be accessed via back(). So instead of AccountList.at(accountListSize - 1) you can write AccountList.back(), although it returns an iterator.

5. If you go through a container, you can use range based loops in C++11, so this

   for (int i = 0; i < accountList.size(); i++) becomes that

   for (Account &account : accountList)

6. Use the available functionality and avoid redeclaring the same thing. For example int accountListSize = AccountList.size(); is a bad design and not needed.

7. You have a couple of functions that are not valid without each other

   void DepositMoney(double depositAmount); void WithdrawMoney(double withdrawalAmount); void SetLastMoneyMovement(int accountID, double amount); void SetBeginningBalance(int accountID); 

   The only way that you can change the last movement is via DepositMoney and WithdrawMoney, so the function SetLastMoneyMovement should not be public, but only be called via those other methods.

8. You seriously lack any form of error checking. What if the user inputs wrong data, are there bounds etc.

9. While your code works, it does not show a valid concept of the problem at hand. The ATM only knows persons and a person has multiple accounts. Therefore, your ATM class should only have a list of available persons and you should create a person class that in turn has the information about that persons accounts. At the same time account balance last transactions are relative to an account and not the person that own that account. So as a stub i would suggest the following design (I ran out of time too :( ):

   class Account { public: Account(const double initialBalance) : balance(initialBalance) {} double GetLastMoneyMovement() const {return lastMoneyMovement;} double GetLastBalance() const {return balance;} double GetBeginningBalance() const {return beginningBalance;} void DepositMoney(const double depositAmount); bool WithdrawMoney(const double withdrawalAmount); private: double balance; double beginningBalance = 0.0; double lastMoneyMovement = 0.0; } class Customer { public: explicit Customer(const std::string &userName, const std::string &passWord) : username(userName), password(passWord) {} void CreateNewAccount(const double initialMoney) {accounts.push_back(Account(initialMoney));} void DepositMoney(const size_t accountID, const double depositAmount); bool WithdrawMoney(const size_t accountID, const double withdrawalAmount); private: const std::string username; std::string password; std::vector<Accounts> accounts; } class AutoTellerMachine { public: AutoTellerMachine() {}; private: std::vector<Customer> customers; } 

Answer 2

Here are my comments:

1. The class is called AutoTellerMachine, but the comment says that it represents a customer who uses the teller machine: this is a bit confusing. If your ATM supports only one operation at a time, then the class name is correct, and the comment is misleading. If it supports multiple concurrent operations, then the name is incorrect.

2. The interface mixes ATM operations and account info. For example, GetAccountBalance, accountBalance etc. would have a better home in a class called Account which would be aggregated by AutoTellerMachine. (This is not the full list of methods/data members)

3. int does not seem the right type for accountLocation. If the variable was meant to be accountId, then the intent becomes clearer, but even then, the type should be size_t or unsigned int.

4. Not sure why accountLocation (or accountId) has a getter and a setter. This exposes implementation details, and breaks encapsulation.

5. ATMs in the real world do not allow creating new accounts, so CreateNewAccount looks out of place.

6. AccountList.at(accountListSize - 1) looks plain wrong - this way the vector will not automatically reallocate the right storage. Consider emplace_back or a similar technique.

7. if(successfulLogin != true) is an odd pattern - why not just say if (successfulLogin) ? Also, why not have just else instead of else if(successfulLogin == true)

I'll stop here (duty calls elsewhere), but hopefully this gives you some useful feedback.