OOP ATM Implementation

Question

I have tried to embody some basic ATM operations in a Object Oriented manner for practice. Would love some review pointers from structure, Object Oriented principles point of view. Also what should be my next steps in order to get good at OO design(I would like to read more code not books/videos, have pushed my mind out of 5 years of Tutorial Hell).

Assumptions - My ATM implementation includes creating a user and open a bank account from an ATM.

ATM Operations-

Account Class

1. Open Account
2. Check Balance
3. Withdraw
4. PIN Change
5. Mini Statement

package templates;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

public class Account {

    private long accountBalance;
    private List<String> miniStatement;
    private boolean accountStatus;
    private String accountPIN;
    
    public Account() {
        
        this.accountBalance = 10000;
        this.accountStatus = true;
        this.accountPIN = "1111";   
        this.miniStatement = new ArrayList<>();
        
    }
    
    public String doWithdraw(long amount) {
        
        if(this.accountStatus) {
            if(getBalance() > 0.00 && amount > 0.00 && getBalance() >= amount) {
                
                this.accountBalance -= amount;
                String str =  "Account debited with $"+amount;
                this.miniStatement.add(str);
                return str;
                
            }else
                return "Amount entered is high, please enter less amount !!! "; 
        }else
            return "Account is already closed"; 
                
    }
    
    public double getBalance() {
        
        if(checkAccountStatus())
            return this.accountBalance;
        return -1.00;
    }
    
    public boolean changePIN(String newPin) {
        
        if(checkAccountStatus()) {
            this.accountPIN = newPin;
            return true;
        }
        return false;
    }
    
    private boolean checkAccountStatus() {
        return this.accountStatus;
    }
    
    public String closeAccount() {
    
        if(checkAccountStatus()) {
            
            String str = "Please collect $"+ getBalance();
    
            this.accountStatus = false;
            
            return str+". Account is now closed";
        }
        return "Account is already closed";
        
    }
    
    public Iterator<String> printMiniStatement() {
        
        return this.miniStatement.iterator();
            
    }
        
}

User Class

1. Create User
2. Set First Name
3. Get First Name
4. Set Last Name
5. Get Last Name
6. Set User Address
7. Get User Address
8. Set Phone Number
9. Get Phone Number

package templates;

public class User {

    private String firstName;
    private String lastName;
    private String address;
    private String phoneNumber;
    
    public User(String firstName,String lastName,String address,String phoneNumber) {
        
        this.firstName = firstName;
        this.lastName = lastName;
        this.address = address;
        this.phoneNumber = phoneNumber;
        
    }
    
    public void setFirstName(String fname) {

        this.firstName = fname;     
    }

    public String getFirstName() {
        return this.firstName;
    }
    
    public void setLastName(String lname) {
        
        this.lastName = lname;
        
    }

    public String getLastName() {
        return this.lastName;
    }
    
    public void setAddress(String addr) {
        
        this.address = addr;
        
    }

    public String getAddress() {
        return this.address;
    }
    
    public void setPhoneNumer(String phonenum) {
        
        this.phoneNumber = phonenum;
        
    }

    public String getPhoneNumber() {
        return this.phoneNumber;
    }
}

Main/Test Class

package test;

import java.util.Iterator;

import templates.*;

public class ATMTest {

    public static void main(String[] args) {
        
        Account savings = new Account();
        
        User user1 = new User("Dummy","Name","27, First Floor, Suok-I","8888888888");
        
        System.out.printf("%s%n", "Account Balance is $"+savings.getBalance());
        
        System.out.println("-------------------------------------");
        
        System.out.printf("%s%n", savings.doWithdraw(50));
        
        System.out.println("-------------------------------------");
        
        System.out.printf("%s%n", savings.doWithdraw(5540));
        
        System.out.println("-------------------------------------");
        
        System.out.printf("%s%n", savings.doWithdraw(3350));
        
        System.out.println("-------------------------------------");        
        
        System.out.printf("%s%n", savings.doWithdraw(1090));
        
        System.out.println("-------------------------------------");
        
        System.out.printf("%s%n", savings.doWithdraw(90));
        
        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.doWithdraw(966));
        
        System.out.println("-------------------------------------");
        
        System.out.printf("%s%n", "Account Balance is $"+savings.getBalance());
        
        System.out.println("-------------------------------------");
        
        savings.changePIN("1611");
        
        Iterator<String> itr = savings.printMiniStatement();
        
        System.out.printf("%20s%n", "MINI STATEMENT");
        
        while(itr.hasNext()) {
            
            System.out.printf("%s%n", itr.next());
            
        }
        
        System.out.println("-------------------------------------");
        
        System.out.printf("%s%n", user1.getFirstName());
        
        System.out.printf("%s%n", user1.getLastName());
        
        System.out.printf("%s%n", user1.getAddress());
        
        System.out.printf("%s%n", user1.getPhoneNumber());
        
        System.out.println("-------------------------------------");
        
        user1.setFirstName("Rain");
        
        user1.setLastName("Man");
        
        user1.setAddress("32, Second Floor, Suok-I");
        
        user1.setPhoneNumer("9999999999");
        
        System.out.printf("%s%n", user1.getFirstName());
        
        System.out.printf("%s%n", user1.getLastName());
        
        System.out.printf("%s%n", user1.getAddress());
        
        System.out.printf("%s%n", user1.getPhoneNumber());
        
        System.out.println("-------------------------------------");
        
        System.out.printf("%s%n", savings.closeAccount());
        
        System.out.println("-------------------------------------");
        
        System.out.printf("%s%n", savings.doWithdraw(1111));
        
        System.out.println("-------------------------------------");
    }

}

Answer 1

Only submit compilable code to review

There is an type-issue in your ATMTest. The printMiniStatement-Method in Iterator<String> itr = savings.printMiniStatement(); has return type void, but the variable is expecting an Iterator. Hence, the code can't be compiled.

Logic & Design

You should introduce some relation between User and Account since they are obviously connected. For example:

• A User may have several Accounts: You can add a field List<Account> accounts to User
• An Account has exactly one User: You can add a field User user to Account

You should add an ATM-Class which is responsible for doing the withdrawal. This should not be done by the Account itself. Take a look at the Single-Responsibility-Principle for more information(link below). You should also add a field String id to Account. It can be used in combination with the accountPin-field to allow an ATM-Instance to conduct some authentication.

As Gilbert pointed out: Don't use floating point when representing money. Floating point representation isn't 100% accurate so you will lose/add a few cents at times. Use an integer/long instead. They don't suffer from precision errors and won't cause these types of bugs that are hard to pin down(link below).

And why does the Account-Class only have a no argument constructor that assigns arbitrary values?

Further reading:

• Single-Responsibility-Principle: https://stackify.com/solid-design-principles/
• Floating point precision error: https://www.simplexacode.ch/en/blog/2018/07/using-bigdecimal-as-an-accurate-replacement-for-floating-point-numbers/

toString-Method

You can override a class' toString-Method so System.out.println(...) can infer a string representation of its instances. Take the following scenario: Creating a new User and immediately printing out the assigned values. You did just that in your ATMTest-class by repeatedly calling different getters. By overriding the User's "toString"-Method, this can be abbreviated to: System.out.println(new User(...))

Further reading:

• https://www.baeldung.com/java-tostring

this-keyword

Only use this when you have to avoid shadowing/ambiguity. Otherwise, it makes your code harder to read.

Further reading:

• When to use this: https://docs.oracle.com/javase/tutorial/java/javaOO/thiskey.html
• Tutorial on shadowing: https://javabeginnerstutorial.com/core-java-tutorial/variable-shadowing/

Whitespace

Use whitespaces to your advantage. Visual gaps created by whitespace are a necessary guide when reading someone's code. They provide additional structure and hence increase readability.

Further reading:

• Code-conventions: https://www.oracle.com/java/technologies/javase/codeconventions-whitespace.html

Wildcard-Import

Statements like import templates.*; import all the classes in the templates-package. Avoid wildcard-imports as they may lead to cumbersome errors when compiling due to naming conflicts.

Further reading:

• https://stackoverflow.com/questions/147454/why-is-using-a-wild-card-with-a-java-import-statement-bad

---

I only added a few comments to your code marking spots where the above points can be implemented. It's running just the way it used to!
Account Class

package templates;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

public class Account {
    /*
    "accountBalance" can be renamed to "balance" since "accountBalance" always belongs to an instance of "Account".
    Preceding "Balance" by "account" doesn't add any information
    */
    private double accountBalance;
    private List<String> miniStatement;
    /*
    "accountStatus" is an extremly broad term and may lead to confusion. If your goal was to mark an account as closed
    consider renaming "accountStatus" to "isClosed"
    */
    private boolean accountStatus;
    private String accountPIN;

    public Account() {
        /*
        the "this"-keyword is only required if you need to avoid ambiguity (this applies to most of your getters/setters as well!!)
        take a look at your "User"-Constructor for more information (I left another comment over there :) )
         */

        this.accountBalance = 10000.0;
        this.accountStatus = true;
        this.accountPIN = "1111";
        this.miniStatement = new ArrayList<>();

    }


    public String doWithdraw(double amount) {
        // Consistency! use either "this.accountStatus" or "checkAccountStatus()"
        if(this.accountStatus) {
            if(getBalance() > 0.00 && amount > 0.00 && getBalance() >= amount) {

                this.accountBalance -= amount;
                // use meaningful names e.g. "substatement"
                String str =  "Account debited with $"+amount;
                this.miniStatement.add(str);
                return str;

            }else
                return "Amount entered is high, please enter less amount !!! ";
        }else
            return "Account is already closed";

    }

    public double getBalance() {

        if(checkAccountStatus())
            return this.accountBalance;

        // why should the balance of an disabled account be -1??
        return -1.00;
    }

    // replace "changePIN" by "setPIN". The terms getter/setter are commonly in OOP. They make your code more readable to other programmers
    public boolean changePIN(String newPin) {
        if(checkAccountStatus()) {
            this.accountPIN = newPin;
            return true;
        }
        return false;
    }

    // replace "checkAccountStatus" by "getAccountStatus"
    private boolean checkAccountStatus() {
        return this.accountStatus;
    }

    public String closeAccount() {

        if(checkAccountStatus()) {
            System.out.printf("%s","Please collect $"+ getBalance());

            this.accountStatus = false;

            return ". Account is now closed";
        }
        return "Account is already closed";

    }

    public void printMiniStatement() {

        if(checkAccountStatus()) {
            Iterator<String> itr = this.miniStatement.iterator();

            System.out.printf("%20s%n", "MINI STATEMENT");

            while(itr.hasNext()) {

                System.out.printf("%s%n", itr.next());

            }
        }

    }

}

User Class

package templates;

public class User {

    private String firstName;
    private String lastName;
    private String address;
    private String phoneNumber;

    /*
    Here, the "this"-keyword was necessary. Otherwise method parameters will shadow the field you want to assign a value to.
     */
    public User(String firstName,String lastName,String address,String phoneNumber) {

        this.firstName = firstName;
        this.lastName = lastName;
        this.address = address;
        this.phoneNumber = phoneNumber;

    }

    /* why abbreviate "firstName" to "fname"? Simply use "firstName" as your method parameter.
       This way you can avoid unnecessary confusion for other programmers. Keep the "this"-keyword to avoid ambiguity
    */
    public boolean setFirstName(String fname) {

        if(!fname.isEmpty()) {
            this.firstName = fname;

            return true;
        }

        return false;

    }

    public String getFirstName() {
        return this.firstName;
    }

    public boolean setLastName(String lname) {

        if(!lname.isEmpty()) {
            this.lastName = lname;

            return true;
        }

        return false;

    }

    public String getLastName() {
        return this.lastName;
    }


    public boolean setAddress(String addr) {

        if(!addr.isEmpty()) {
            this.address = addr;

            return true;
        }

        return false;

    }

    public String getAddress() {
        return this.address;
    }

    public boolean setPhoneNumer(String phonenum) {

        if(phonenum.length() == 10) {
            this.phoneNumber = phonenum;

            return true;
        }

        return false;

    }

    public String getPhoneNumber() {
        return this.phoneNumber;
    }
}

ATMTest Class

package test;

import java.util.Iterator;

// avoid wildcard imports
import templates.*;

public class ATMTest {

    public static void main(String[] args) {

        Account savings = new Account();

        User user1 = new User("Dummy","Name","27, First Floor, Suok-I","8888888888");

        System.out.printf("%s%n", "Account Balance is $"+savings.getBalance());

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.doWithdraw(50));

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.doWithdraw(5540));

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.doWithdraw(3350));

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.doWithdraw(1090));

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.doWithdraw(90));

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.doWithdraw(966));

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", "Account Balance is $"+savings.getBalance());

        System.out.println("-------------------------------------");

        savings.changePIN("1611");

        /*
        // the following line can't be compiled -> incompatible types
        Iterator<String> itr = savings.printMiniStatement();

        System.out.printf("%20s%n", "MINI STATEMENT");

        while(itr.hasNext()) {

            System.out.printf("%s%n", itr.next());

        }
        */

        System.out.println("-------------------------------------");

        // use the "toString"-Method here to print "user1"
        System.out.printf("%s%n", user1.getFirstName());

        System.out.printf("%s%n", user1.getLastName());

        System.out.printf("%s%n", user1.getAddress());

        System.out.printf("%s%n", user1.getPhoneNumber());

        System.out.println("-------------------------------------");

        user1.setFirstName("Rain");

        user1.setLastName("Man");

        user1.setAddress("32, Second Floor, Suok-I");

        user1.setPhoneNumer("9999999999");

        System.out.printf("%s%n", user1.getFirstName());

        System.out.printf("%s%n", user1.getLastName());

        System.out.printf("%s%n", user1.getAddress());

        System.out.printf("%s%n", user1.getPhoneNumber());

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.closeAccount());

        System.out.println("-------------------------------------");

        System.out.printf("%s%n", savings.doWithdraw(1111));

        System.out.println("-------------------------------------");

    }

}
```