Simple bank application in Java

Question

I wrote a simple Java bank application and I would like to get an review! I learned today about exceptions, so I tried to apply it on my code, but sadly I couldn't made it. I hope someone here could help me with that.

Account.java

private final String firstName;
private final String lastName;
private double balance;
private static int uid = 0;
private final String phoneNumber;
private final int id;
public Account(String firstName, String lastName, String phoneNumber){
 this.firstName = firstName;
 this.lastName = lastName;
 this.phoneNumber = phoneNumber;
 this.balance = 0.0;
 uid++;
 this.id = uid;
}
public String getFirstName(){
 return this.firstName + "";
}
public String getLastName(){
 return this.lastName + "";
}
public double getBalance(){
 return this.balance;
}
public void setBalance(double newBalance) {
 this.balance = newBalance;
}
public int getID(){
 return this.id;
}
public String getPhoneNumber(){
 return this.phoneNumber + "";
}
public void depositMoney(double depositAmount){
 this.balance += depositAmount;
 System.out.println("You have deposit " +depositAmount +" to your account." + "\n" +
 "Balance is now: " +this.balance);
}
public void withdrawal(double withdrawalAmount){
 if(this.balance < withdrawalAmount) {
 System.out.println("You don't have enough funds.");
 } else {
 this.balance -= withdrawalAmount;
 System.out.println("You have withdrawal " +withdrawalAmount + " from your account." + "\n" +
 "Balance is now: " +this.balance);
 }
}
public void moneyTransfer(Account thisAccount, Account toAccount, double amountToTransfer){
 if(thisAccount.getBalance() > 0) {
 toAccount.setBalance(toAccount.balance += amountToTransfer);
 thisAccount.setBalance(this.balance -= amountToTransfer);
 } else {
 System.out.println("You don't have enough funds");
 }
}
@Override
public String toString(){
 return "Name: " + getFirstName() + "\n" +
 "Last name: " +getLastName() +"\n" +
 "Balance: " + getBalance() + "\n" +
 "ID: " + getID();
}

Bank.java

private final List<Account> bankAccounts;
private final Scanner sc;
public Bank() {
 bankAccounts = new ArrayList<>();
 sc = new Scanner(System.in);
}
public Account isAccountExist(int accountID, String phoneNumber) {
 for (Account account : bankAccounts) {
 if (account.getID() == accountID && account.getPhoneNumber().equals(phoneNumber)) {
 return account;
 }
 }
 System.out.println("One of the details is incorrect");
 return null;
}
//overload method -
public Account isAccountExist(String phoneNumber) {
 for (Account account : bankAccounts) {
 if (account.getPhoneNumber().equals(phoneNumber)) {
 return account;
 }
 }
 System.out.println("One of the details is incorrect");
 return null;
}
public void registerAccount() {
 System.out.println("First name?");
 String firstName = sc.next();
 System.out.println("Last name?");
 String lastName = sc.next();
 System.out.println("Phone number?");
 String phoneNumber = sc.next();
 if (isPhoneNumberCorrect(phoneNumber)) {
 bankAccounts.add(new Account(firstName, lastName, phoneNumber));
 System.out.println("You have created account successfully!" + "\n" +
 "Your account ID is: " + bankAccounts.get(bankAccounts.size() - 1).getID());
 }
}
public void loginAccount() {
 System.out.println("Please enter your ID:");
 int accountID = sc.nextInt();
 System.out.println("Please enter your phone number:");
 String phoneNumber = sc.next();
 if (isPhoneNumberCorrect(phoneNumber)) {
 Account selectedAccount = isAccountExist(accountID, phoneNumber);
 boolean exitRequested = false;
 while (!exitRequested) {
 PrintService.existAccountMenu();
 int choice = Integer.parseInt(sc.next());
 switch (choice) {
 case 1:
 System.out.println(selectedAccount.toString());
 break;
 case 2:
 System.out.println("Please enter deposit amount:");
 double depositAmount = sc.nextDouble();
 selectedAccount.depositMoney(depositAmount);
 break;
 case 3:
 System.out.println("Please enter withdrawal amount:");
 double withdrawalAmount = sc.nextDouble();
 selectedAccount.withdrawal(withdrawalAmount);
 break;
 case 4:
 System.out.println("Please enter the phone number of the account you want to transfer to: ");
 String accountPhoneNumber = sc.next();
 if (isPhoneNumberCorrect(accountPhoneNumber)) {
 Account accountToTransfer = isAccountExist(accountPhoneNumber);
 System.out.println("Enter the amount of money you would like to transfer:");
 double moneyToTransfer = sc.nextDouble();
 selectedAccount.moneyTransfer(selectedAccount, accountToTransfer, moneyToTransfer);
 break;
 }
 case 5:
 exitRequested = true;
 break;
 default:
 System.out.println("Wrong input");
 break;
 }
 }
 }
 }
public static boolean isPhoneNumberCorrect(String phoneNumber){
 if(phoneNumber.length() != 10){
 System.out.println("Phone number must be 10 digits.");
 return false;
} else {
 return true;
 }
}

PrintService.java

public static void printMenu() {
 System.out.println("Hello, press: " + "\n" +
 "\r" + "1.Register" + "\n" +
 "\r" + "2.Log in" + "\n" +
 "\r" + "3.Exit");
}
public static void existAccountMenu(){
 System.out.println("What would you like to do?" + "\n" +
 "\r" + "1.Info" + "\n" +
 "\r" + "2.Deposit money" + "\n" +
 "\r" + "3.Withdrawal money" + "\n" +
 "\r" + "4.Money transferring" + "\n" +
 "\r" + "5.Exit");
}

Main.java

public static void main(String[] args) {
 Scanner sc = new Scanner(System.in);
 Bank bank = new Bank();
 boolean exitRequested = false;
 while(!exitRequested) {
 PrintService.printMenu();
 int choice = Integer.parseInt(sc.next());
 switch (choice) {
 case 1:
 bank.registerAccount();
 break;
 case 2:
 bank.loginAccount();
 break;
 case 3:
 exitRequested = true;
 break;
 default:
 System.out.println("Wrong input");
 break;
 }
 }
}

• forsvarir: It's easier to review your code if you include the class definitions from your source files. It means there's less work required if somebody wants to run your code locally.
• TheEnvironmentalist: Hi! Biggest warning is around race conditions. When it comes to banks, "correctness" is very important, otherwise money could appear or disappear. Your depositMoney, withdrawal, and moneyTransfer methods are not thread-safe, and in all three money can appear or disappear if used concurrently

Answer 1

Than you for sharing your code.

I think you did a good job and there are only minor things to mention:

odd property manipulations

In some of your getter method you add an empty String ("") to the property value. Why do you do that? The String returned does has the same content but a new object is created. Is that really needed or intended?

Naming

The most important rule when finding names is "don't surprise your readers".

isAccountExist()

By the Java Naming Conventions a method (or variable) that begins with is, has, can or alike returns or holds a boolean (true or false).

By the same conventions a method starting with get simply returns a member variable and does not contain any processing logic. Therefore getAccount wouldn't be appropriate either.

When we look at that method, then you lookup an account and therefor the method should be named lookupAccount instead.

moneyTransfer()

To improve readability methods always should start with a verb (a "do"-word). Therefore this Method should be namend transferMoney. Compare that:

 bank.moneyTransfer(fromAccount,toAccount);
 bank.transferMoney(fromAccount,toAccount);

Doesn't read the second line more like an instruction?

loginAccount()

This method name is another "lie". You don't just login but you start managing the account. The method name should reflect that.

• John Glen: Probably a non english speaker?
• Timothy Truckle: @JohnGlen yes, you're right.
• עמית שוקרון: not my main language ;)

Answer 2

You have bugs

• A malicious user can enter negative numbers to withdraw negative numbers, this would result in adding money to their account.
• A malicious user can also enter negative numbers to "steal" money from another account when transferring money with such negative number.

Create specific types for your business objects

Specifically, create a Money class which can add, subtract other Money objects. Internally to Money, you should use a BigDecimal to represent the amounts. But certainly not a double or a float because of their lack of precision. Try adding 0.1 and 0.2. You won't get 0.3: you'll get 0.30000000000000004. BigDecimal avoids falling into that trap, and if you use add 0.1 and 0.2, you'll really get 0.3 and not any other number.

Account should contain a balance represented by a Money, but that balance should never go in the negative.

Create a PhoneNumber class. This way, you can lookup by phone number with an appropriate object. The validation will happen only when you create the object, and internally you can represent a PhoneNumber as you wish, but I recommend a String, as you used. Here, since you check if a phone number exists, don't forget to reimplement equals and hashcode. Your IDE should help you make those methods correctly.

Use exceptions in your code

You asked to use exception, so just use them.

What are the possible issues in your code?

1. That an account doesn't exist.
2. That an amount is negative to avoid bugs.
3. That an amount is bigger than the balance when trying to withdraw.

So let's just write those exception:

UnknownAccountException.java

public class UnknownAccountException extends RuntimeException {
 public UnknownAccountException(String message) {
 super(message);
 }
}

NegativeAmountException.java

public class NegativeAmountException extends RuntimeException {
 public NegativeAmountException(String message) {
 super(message);
 }
}

InsufficientBalanceException.java

public class InsufficientBalanceException extends RuntimeException {
 public InsufficientBalanceException(String message) {
 super(message);
 }
}

Your actions should be simple

Here your actions are complex. They do more than one thing. If you deposit money, not only do you add money there, but you also print the balance afterwards. A method named depositMoney should only deposit money, so avoid printing the balance. If at all, print the balance in the UI, that is in your case, your main class.

Also, you should keep your actions consistent. If you create a depositMoney, why don't you use it when you transfer money?

Your account's money-related methods should be as simple as this:

private void checkPositiveAmount(Money amount) {
 if (amount.asBigDecimal().compareTo(BigDecimal.ZERO) < 0) {
 throw new NegativeAmountException("Negative amount: " + amount);
 }
}
private void checkSufficientFunds(Money amount) {
 if (balance.compareTo(amount) < 0) {
 throw new InsufficientBalanceException("Not enough funds to withdraw: " + amount);
 }
}
public void depositMoney(Money amount) {
 checkPositiveAmount(amount);
 balance = balance.add(amount);
}
public void withdrawMoney(Money amount) {
 checkPositiveAmount(amount);
 checkSufficientFunds(amount);
 balance = balance.subtract(amount);
}
public void transferMoneyTo(Money amount, Account destination) {
 withdrawMoney(amount);
 destination.depositMoney(amount);
}

With such simple actions, the risk of errors is way lower than previously.

Your UI is too complex

Your UI contains few methods, and those are so big that you don't know how to name those methods. Also, you make several errors in your main interface.

Reduce your UI in small methods. Small methods means less chance to mess up.

Here's how I'd decompose your Main class:

public class Main implements Runnable {
 public static void main(String[] args) {
 new Main().run();
 }
 private final Scanner scanner = new Scanner(System.in);
 private final Bank bank = new Bank();
 @Override
 public void run() {
 do {
 int option = selectAccountOption();
 switch (option) {
 case 1:
 Account account = register();
 manageAccount(account);
 break;
 case 2:
 Account account = login();
 manageAccount(account);
 break;
 case 3:
 exit();
 return;
 }
 } while (true);
 }
 
 private int selectAccountOption() {
 do {
 System.out.println("Hello, press: ");
 System.out.println("1. Register a new account");
 System.out.println("2. Log in to your account");
 System.out.println("3. Exit");
 int option = scanner.nextInt();
 if (1 <= option && option <= 3) {
 return option;
 }
 System.out.println("Your input was not recognized. Please try again.");
 } while (true); 
 }
 private Account register() {
 String firstName = readFirstName();
 String lastName = readLastName();
 PhoneNumber phoneNumber = readPhoneNumber();
 Account account = new Account(firstName, lastName, phoneNumber);
 printAccountId(account);
 return account;
 }
 private Account login() {
 PhoneNumber phoneNumber = readPhoneNumber();
 Account account = lookupAccount(phoneNumber);
 return account;
 }
 private void manageAccount(Account account) {
 do {
 printBalance(account);
 int option = selectManageAccountOption();
 switch (option) {
 case 1:
 info(account);
 break;
 case 2:
 deposit(account);
 break;
 case 3:
 withdraw(account);
 break;
 case 4:
 transfer(account);
 break;
 case 5:
 return;
 }
 } while (true);
 }
 private Account lookupAccount(PhoneNumber phoneNumber) {
 ...
 }
 private Account lookupAccount(PhoneNumber phoneNumber, int id) {
 ...
 }
 
 private void info(Account account) {
 ...
 }
 private void deposit(Account account) {
 Money amount = readAmountToDeposit();
 try {
 account.depositMoney(amount);
 } catch(NegativeAmountException e) {
 System.out.println("Illegal amount. The amount was not deposited.");
 }
 }
 private void withdraw(Account account) {
 Money amount = readAmountToWithdraw();
 try {
 account.withdrawMoney(amount);
 } catch (NegativeAmountException e) {
 System.out.println("Illegal amount. The amount was not withdrawn");
 } catch (InsufficientBalanceException e) {
 System.out.println("Amount too high. The amount was not withdrawn.");
 }
 }
 
 private void transfer() {
 PhoneNumber phoneNumber = readPhoneNumber();
 Account destination = lookupAccount(phoneNumber);
 Amount amount = readAmountToTransfer();
 try {
 account.transferMoney(amount, destination);
 } catch (NegativeAmountException e) {
 System.out.println("Illegal amount. The amount was not transferred");
 } catch (InsufficientBalanceException e) {
 System.out.println("Amount too high. The amount was not transferred.");
 }
 }
}

There are still plenty of methods, but I tried to have one type of each method used, so you should be able to replicate those to fill the missing methods.

• pacmaninbw: This is a good answer, but in the future rather than use you or yours it would be better to just refer to the code. Sometimes you can seem like a personal attack.

Answer 3

Don't use floats or doubles for money/currency.

https://stackoverflow.com/questions/3730019/why-not-use-double-or-float-to-represent-currency

Naming

As Timothy pointed out, methods starting with is are expected to return a boolean value.

A more readable name for isAccountExist would be isExistingAccount.

For depositing depositMoney is used, but for withdrawing withdrawel is used. For consistency it would be better to rename withdrawel to withdrawMoney.

Concurrency

The code does things that have to do with money. When writing software that relates to money (or anything with transactions), it is important to think about things like thread-safety. Java has things in it's standard library to help with this. A few examples of this are atomic variables and synchronization.

Exception handling

As you mentioned you didn't have time to implement exception handling. From a quick look at the code two types of functions which can throw an exception are used. These are the next and parse functions.

When sc.nextInt() is called and no input is given it will throw a NoSuchElementException. If the input cannot be parsed as an integer it will throw an InputMismatchException. A try/catch block can be used to handle these exceptions and prevent them from crashing your application.

Without exception handling:

int accountID = sc.nextInt();

With exception handling:

int accountID = 0;
while (true) {
 try {
 accountID = sc.nextInt();
 break;
 } catch (NoSuchElementException e) {
 sc.nextLine();
 }
}

In this example the code will try to read the input and parse it as an integer. If the input is an integer accountID will be set and the code will break from the while loop. If a NoSuchElementException or an InputMismatchException occurs the code will go to the next line for input and try again. Only the NoSuchElementException has to be caught because InputMismatchException is a subclass of NoSuchElementException.

The Integer.parseInt() function will throw a NumberFormatException if the input cannot be parsed. In a few instances Integer.parseInt() can actually be avoided by using the sc.nextInt() function like in other parts of the code.

int choice = Integer.parseInt(sc.next());

Could be replaced to avoid extra exception handling by:

int choice = sc.nextInt();

Answer 4

Given the scope of what you're trying to create, I would have only a few pieces of advice:

Use streams to find things in arrays

Bank#isAccountExists could be re-written to something like:

return bankAccounts.stream()
 .filter(account -> 
 account.getID() == accountID && account.getPhoneNumber().equals(phoneNumber)
 )
 .findFirst().get();

(code may be a bit wrong, I'm going from memory)

why? - because once you can read streams it's easier to understand and reason about what's going on.

Keep cognitive complexity low

Bank#loginAcount is long and has multiple layers (for example the line Account accountToTransfer = isAccountExist(accountPhoneNumber); is five layers deep, being executed only if the phone number is correct, exit hasn't been requested, the choice is '4' and the phone number is correct (wait.. did you just check the same condition twice?)). You can reduce complexity in this case by creating private methods to do logical subsections of the work. For example, the logic under case 4 could be it's own method, called transferToAccount and everything inside the while loop could be in a method called handleUserChoice.

why? - good code is code that's easy to read. Complex methods make for hard to read code.

Separate data from UI input

The Bank#loginAcount method is UI processing, which doesn't really belong in a class dedicated to representing part of your data (Bank). I think you should separate these two so you can reason about them separately.

why? - separation of concern: you should separate code into pieces that make it easier to change one without the other.

Other

@Olivier Grégoire gave some advice on using exceptions, although I think your use-case is too simple to need them.

@Timothy Truckle gave some good advice on naming conventions, particularly isAccountExist shouldn't start with 'is' as it's not returning a boolean.

Extra advice

Use sonar lint. It will give you a lot of good advice!