OOP ATM application

Question

As part of my studies, I need to gather peer feedback as part of my object-oriented ATM application. I have only used classes (inheritance etc.) and no GUI is required for this. Could you please give me feedback in terms of coding and how I could improve it?

Requirements:

• Object-oriented application using Java
• Application that models a standard ATM and an ATM capable of handling deposits/customer accounts (using inheritance)
• ATM will display the balance (which shows in Standard ATM)
• A withdrawal of up to 300ドル per day can be made (depending on the balance)
• A change of PIN (struggling with this one so didn't implement it)
• Deposit ATM should allow deposits of up to 100ドル a day
• Both ATM’s prevent withdrawal from taking place if the balance is insufficient
• Customer can access the services once the correct combination of account/PIN number has been made
• After three consecutive attempts of incorrect account/PIN combination, the account is disabled

customer.java (ArrayList)

import java.util.ArrayList;
import java.util.List;
public class Customer {
private String acctNo;
private String PIN;
private double balance;
private int attempt;
public Customer(String acctNo, String PIN, double balance, int attempt) {
 super();
 this.acctNo = acctNo;
 this.PIN = PIN;
 this.balance = balance;
 this.attempt = attempt;
}
public int getAttempt() {
 return attempt;
}
public void setAttempt(int attempt) {
 this.attempt = attempt;
}
public String getAcctNo() {
 return acctNo;
}
public void setAcctNo(String acctNo) {
 this.acctNo = acctNo;
}
public String getPIN() {
 return PIN;
}
public void setPIN(String PIN) {
 this.PIN = PIN;
}
public double getBalance() {
 return balance;
}
public void setBalance(double balance) {
 this.balance = balance;
}

Deposit ATM

 public class DepositAtm extends StandardAtm { //subclass //variables used 
 from superclass: StandardATM
 private Customer cust1;
 private double balance;
//constructor
public DepositAtm(Customer customer) 
 {
 super(customer);
 this.cust1 = customer;
 this.balance = balance;
 }
public void depositCash(int amount) 
{
 if (amount > 0 && amount < 100 && cust1.getBalance() >= amount) 
 { 
 if (cust1.getBalance() >= amount)//compare balance to the amount to deposit + amount = balance(amount) + amount;
 { 
 cust1.setBalance(cust1.getBalance()+amount); //set the new balance
 System.out.println("Deposit Complete. New Balance: £"+ cust1.getBalance());
 }
 } else 
 {
 System.out.println("Transaction cancelled due to insufficient funds. Please retry:"); 
}
}

Standard ATM

import java.util.ArrayList; import java.util.List;
public class StandardAtm { //declaration of variables
private Customer cust1;
List<Customer> cust = new ArrayList<>(); //an list for number of customers
Boolean accessed = false;
//constructor
public StandardAtm (Customer customer) {
 this.cust1 = customer;
}
public void displayBalance() 
{ 
 if (accessed = true) 
 {
 System.out.println("Balance: "+cust1.getBalance()); //displays the balance
 }
}
public void accessacc(String acctNo, String PIN) //log in method
{ 
 if (cust1.getAcctNo().equals(acctNo) && cust1.getPIN().equals(PIN)) 
 {
 System.out.println("Welcome! Your account balance is: £"+cust1.getBalance());
 } 
 else 
 { 
 //for (int attempt=1; attempt<=3;attempt++) //1. limit the number of attempts 
 /*if (attempt == 3 && cust1.getAcctNo().equals(acctNo) != cust1.getPIN().equals(PIN))
 { //1. find the opposite of equals
 System.out.println("Invalid Account Number & PIN! " + "Attempt: "+attempt);
 }*/
 if (cust1.getAcctNo().equals(acctNo) != cust1.getPIN().equals(PIN))
 cust1.setAttempt(cust1.getAttempt()+1);
 { 
 System.out.println("Invalid Account Number & PIN! " + "Attempt: "+ cust1.getAttempt());
 if (cust1.getAttempt() >= 3) 
 {
 System.out.println("Number of attempts exceeded. Please contact your bank. ");
 }
 }
 }
}
public void withdrawCash(int amount) 
{ 
 if (accessed=true) 
 { 
 if (amount > 0 && amount < 301 && cust1.getBalance() >= amount) //nested statements are required to run a number of commands
 { 
 if(amount > 0 && amount < 301) 
 { 
 if (cust1.getBalance() >= amount) //compare balance to the amount you want to withdraw if amount to withdraw is same as balance
 {
 cust1.setBalance(cust1.getBalance()-amount); //set the new balance
 System.out.println("Withdrawn complete! New Balance : £"+ cust1.getBalance());
 }
 } else 
 {
 if (accessed = false) 
 System.out.println("Transaction cancelled: please enter correct amount"); 
 }
 }
}

Test Drive

public class Testdrive {
public static void main(String[] args) {
 Customer cust1 = new Customer("123456", "1224", 50, 0); // New customer object
 StandardAtm SA1 = new StandardAtm(cust1);//created a standard ATM object and passed cust1 in it
 SA1.accessacc("123456", "1224");//given SA1 an account number: the printout method should not work
 //SA1.withdrawCash(100);
 DepositAtm SA2 = new DepositAtm(cust1); // created a deposit ATM object and passed cust1 through it
 SA2.depositCash(50);
}

• Sat Kaur: Thank you. Will implement this and bear in mind in future projects.

Answer 1

Let's list the comments by order of severity:

Bugs

There are several that I could see:

1. this if statement if (cust1.getAcctNo().equals(acctNo) != cust1.getPIN().equals(PIN)) surely does not do what is intended of it. Moreover, it is redundant because you already ask on equality of the acct and pin. so in the else clause you know that at least one of these fields is not the correct value.

2. You never set the accessed variable to true.

3. The balance instance variable of the DepositATM is never used beyond the constructor.

4. The cust1 instance variable is declared private. This means that each of the two ATM classes has its own private variable. Not that I understand what is it for, because -

5. Your list of customers never gets populated. In fact, in the current state, an instance of an ATM machine can handle just one customer. The list is also names cust which is misleading. at the very least it should be something like customers

6. The two variables that are shared between the two ATM classes (the list and the boolean) are declared in the default "package private" accessor. Altough it will work (they can be accessed and modified by the two ATM classes) this means that classes that are not ATM but are in the same package can accessed these variables too. The variables should be declared protected in order for them to be shared only by the ATM hierarchy tree.

7. The nested if statements in withdrawCash() are redundant. Perhaps the comment was an attempt for an explanation but it is neither clear nor correct.

8. The exercise stated that the withdrawal and deposit limits are per day. This is not addressed in the solution.

Design

As was mentioned in the bugs section, the design of the ATM requires that you create a new ATM instance for every customer. In order to fix this, let us imagine how a real ATM probably works:

One ATM machine can serve customers from multiple banks. When a customer enters the card and a PIN number, the ATM identifies the bank and sends these details to it. The Bank holds the list of its customers and is responsible for authenticating them.

After the authentication process is over, the ATM will get a Customer instance. The ATM will use this instance to do the balance queries and modifications.

At the end of the process, when the customer gets his/her credit card back, the ATM machine should send the Customer object back to the bank so it can update its internal records.

Translating this into OO design. We have the following Classes:

a Customer class much like the one you have.
a Bank class that holds a List (or any other collection/data structure) of Customers. The Bank exposes a getCustomer(int acctNo, int PIN) and updateCustomer(Customer cust) methods. the getCustomer also does the authentication. the ATM class gets a List (or any other collection/data structure) of Banks in the constructor and should be able to identify the bank from the customer's account number. We can say that the first two digits are the bank ID.

this design is both robust and modular. It allows each bank to do their own book keeping, customer authorization etc. If this looks like an overkill, then You can say that the ATM handles customers from one bank (make sure to mention this in a comment so it is known that you didn't omit this considearion). In this case, an ATM should maintain the customers list (probably get the list in the constructor).

Best Practices

1. Do not use literals and constants in the code. Each literal should be defined as a static final variable. The reasons for this are 1) it gives the literal a name and meaning and 2) there is only one place where the literal value is specified, making it easier to modify the value. example: public static final int MAX_DAILY_WITHDRAWAL = 300;

2. You do not have to initialize all instance variables in the constructor. Some of them may have default values. A new customer will always have zero attempts so why specify this value each time you create a new instance?

3. The accessed variable is declared as Boolean which is an Object and not the primitive boolean type. This means that each time you assign a value or compare the variable to a value, there are additional actions that need to be done to access and modify the variable. It is more efficient to declare the variable as the primitive boolean type.

• Sharon Ben Asher: If you are happy with my answer, I would appreciate if you would accept it.
• Sat Kaur: done! If anyone else would like to comment, please feel free to.