Bank app in Java

Question

I am a beginner in java. I just made this app and i am looking for other developers opinion. What do you think about my code structure? What can i do more to improve this app.

public class Account {

    String name;
    int balance = 1000;

    //construct
    public Account(String myName, int myBalance) {
        this.name = myName;
        this.balance = myBalance;
    }

    public String getName() {
        return name;
    }

    public int getBalance() {
        return balance;
    }

    public String displayBalance(int pinNumber) {
        if (pinNumber == myPinCode()) {
            return "Your balance is " + balance + " dollars";
        } else {
            return pinError();
        }
    }

    public String deposite(int pinNumber, int amount) {
        if (pinNumber == myPinCode()) {
            balance += amount;
            return "you deposite " + amount + " and your balance is now " + balance + " dollars";
        } else {
            return pinError();
        }

    }

    public String withdraw(int pinNumber, int amount) {
        if (pinNumber == myPinCode()) {
            balance -= amount;
            return "you withdraw " + amount + " and your balance is now " + balance + " dollars";
        } else {
            return pinError();
        }

    }

    private int myPinCode() {
        int myPin = 1234;
        return myPin;
    }

    private String pinError() {
        String errorMessage = "You have enter the rong pin number!";
        return errorMessage ;
    }


    public static void main(String[] args) {
        Account person1 = new Account("Emerson", 10000);
        String myName = person1.getName();
        String myAccountBalance = person1.displayBalance(1234);
        String newAccountBalance = person1.deposite(1234,500);
        String withdrawFromAccount = person1.withdraw(1234,250);

        System.out.println(myName);
        System.out.println(newAccountBalance);
        System.out.println(withdrawFromAccount);
    }

}

Answer 1

Your variables are not set to private. That means that anyone in the same package as well as subclasses can change the account's balance, without it knowing about it.

---

Instead of thinking about a bank account as just a balance and some operations on it, regard it as a list of deposits and withdrawals, from which you calculate your current balance. In a real bank account, you can look into the history of operations done, and even undo them in some cases.

---

A bank account does not have a name. Instead the customer has a name, to which the account belongs. A customer might have multiple bank accounts.

---

Your bank account only supports full dollars as balance. It cannot have 13 dollars and 37 cents, for example. Note however that money should not be represented as floating point numbers. Instead you should use an int (or long) to represent the cents, and then calculate dollars and cents from that. In a real banking application, you might even use a millionth of a cent as your normalized scale.

---

The account should not need a method to display the balance. You already have getBalance(), and the ATM is responsible for displaying it.

Furthermore, displayBalance does not display the balance. Instead it returns a string. Method names should describe what the method does (or sometimes what it returns) as precisely as possible, but they should never say that they do something that they are actually not doing.

---

Instead of

int myPin = 1234;
return myPin;

Just do

return 1234;

That's shorter and more readable. However in this case, it would actually make more sense for it to be a private field.

---

You are returning strings that are named errors. However it is not really an error. An error is something more severe. You might call it an error if the ATM cannot connect to the network. But that's not the account's job.

What the method pinError returns is actually an incorrectPinMessage. Like with the PIN, this could just be a private field, because the method does not really do anything. Making it a field also enables you to change the message more easily if you decide that a different message would be more appropriate.

Also the account should not really know about the message that is displayed when the customer inputs the wrong PIN. That is also the ATM's job.

---

The account should not have the main method. Instead the main method should be in the main class of the banking software, and the account should be instantiated and used by it.

---

My version

Following is my version of your bank account class, with the members removed that I think should not be part of the account class. Furthermore I added an interface Transaction with two implementations Withdrawal and Deposit. Both are immutable (meaning they cannot be changed, neither through direct access nor methods). Note that they are not implemented as what you would usually regard as transactions, but it should give a hint about in which direction the class could go.

public interface Transaction {
    public int getDeltaCents();
}

public class Withdrawal implements Transaction {
    private int deltaCents;

    public Withdrawal(int cents) {
        deltaCents = - cents;
    }

    public int getDeltaCents() {
        return deltaCents;
    }
}

public class Deposit implements Transaction {
    private int deltaCents;

    public Withdrawal(int cents) {
        deltaCents = cents;
    }

    public int getDeltaCents() {
        return deltaCents;
    }
}

public class Account {
    private int pin;
    private List<Transaction> transactions = new ArrayList<Transaction>();

    public Account(int pin) {
        this.pin = pin;
    }

    // Allow reading balance without PIN, but not writing.
    public int getBalanceCents() {
        int balance = 0;
        for (Transaction t : transactions) {
            balance += t.getDeltaCents();
        }
        return balance;
    }

    // Return a boolean instead of a string.
    // Let the application handle a wrong PIN.
    public boolean deposite(int pin, int cents) {
        if (this.pin != pin) { return false; }
        Transaction t = new Deposit(cents);
        transactions.add(t);
        return true;
    }

    // Return a boolean instead of a string.
    // Let the application handle a wrong PIN.
    public boolean withdraw(int pin, int cents) {
        if (this.pin != pin) { return false; }
        Transaction t = new Withdrawal(cents);
        transactions.add(t);
        return true;
    }
}

Answer 2

Just a few quality-of-life changes off the top of my head:

Set pin, error message values as private final members within the class, instead of hard-coding them within methods.

You can also change all returned strings to private class members with placeholders, then call them in their respective methods using String.format():

So instead of:

return "Your balance is " + balance + " dollars";

You could have:

private final String BALANCE_MESSAGE = "Your balance is {} dollars."; // place at the top of your class next to other members.
// ... 
return String.format(BALANCE_MESSAGE, balance); // new return value in your displayBalance() method

This would clean up your methods a bit.

Answer 3

Some suggestions nested as comments in the code below:

// are you fine subclassing this Account class ? if not then declare this class as final.
public class Account {

    private final String name;
        // variable should be private. declaring variables final forces you to initialize it in constructor.
    private double balance = 1000;  // add a comment that default value would be 1000. Also consider a double datatype.
    private final int pin;

    // add final to param.
    // name & balance are more professional naming than 
    public Account(final String name, final double balance, final int pin) {
        this.name = name;
        this.balance = balance;
        // checks if pin has only 4 digits. else throw an exception.
        this.pin = pin;
    }

    public String getName() {
        return name;
    }

    //    
    //    public double getBalance() {
    //        return balance;
    //    }

    public String displayBalance(int pinNumber) {
        if (pinNumber == pin) {
            return "Your balance is " + balance + " dollars";
        } else {
            return pinError();
        }
    }

    // typo in spelling deposite
    // use synchronized to guard against race conditions.
    public synchronized String deposit(int pinNumber, double amount) {
        if (pinNumber == pin) {
            balance += amount;
            return "you deposite " + amount + " and your balance is now " + balance + " dollars";
        } else {
            // throw an exception instead of returning a string.
            return pinError();
        }

    }

    // use synchronized to guard against race conditions.
    public synchronized String withdraw(int pinNumber, double amount) {
        if (pinNumber == pin) {
            balance -= amount;
            return "you withdraw " + amount + " and your balance is now " + balance + " dollars";
        } else {
            // throw an exception instead of returning a string.
            return pinError();
        }

    }
    //    you dont need this method.
    //    private int myPinCode() {
    //        int myPin = 1234;
    //        return myPin;
    //    }

    private String pinError() {
        // you could have a static variable storing this string.
        return "You have enter the wrong pin number!";
    }

}