HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

ATM (Automated Teller Machine) Terminal Application

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
terminalapplicationautomatedtellermachineatm

Problem

I consider myself a beginner in Java. Therefore I want to improve and learn about Java(8) and OOP as much as I can. Feedback about:

  • performance



  • clearn code



is very much appreciated.

If you could review with regard to these questions, then I'd be very grateful:

  • Is having multiple returns in the function pinIsValidFor a bad pattern? How would you write it differently?



  • Are the classes too long (they nearly got 300 lines of code)? What would be a good size for a Java-class



This is "Automated Teller Machine Terminal Application" in Java:

You can do typical transaction like deposit, withdraw, and show balance. It persistently saves your transactions in an external file so you can see your changes after restarting the application.

It also (tries to) handle all exception cases.

GitHub

Main file (not much in here)

import controller.Atm;

public class Main {
    public static void main(String[] args)  {
        Atm.start();
    }
}


In the model folder I got my AccountCard file:

```
package model;

import model.exceptions.DepositNegativeBankTransfer;
import model.exceptions.ExceedLimitTransfer;
import model.exceptions.OverdrawBankTransfer;
import model.exceptions.ZeroBankTransfer;

import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.Map;

public class AccountCard {
private String pin = null;
private int amount = 0;
DateFormat dateFormat = new SimpleDateFormat("yyyy/MM/dd HH");

private Map transferHistory = new DefaultLinkedHashMap<>(0);

public AccountCard(String pin, int amount, String withdrawDate, int timesWithDraw) {
this.pin = pin;
this.amount = amount;
this.transferHistory.put(withdrawDate, timesWithDraw);
}

public void setPin(String pin) {
this.pin = pin;
}
public String getPin() {return this.pin;}

public boolean isPinCorrect(String pinInputByUser) {
return this.pin.equals(pinInputByUser);
}
publ

Solution

Thanks for sharing the code.

Here is what I think about it:

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

On the bright side you follow the Java Naming Conventions.

But in Java there is an exception for boolean variables and methods: they should start with is or has like instead of atmIsActivated it should be isAtmActivated.

Entry class.

In general it is a good thing to have a tiny entry class. We usually use it to switch from static context to object context.

Your entry class simply delegates to another static method in a different class. Therefore it is almost useless.

OOP

Doing OOP means that you follow certain principles which are (amongst others):

  • information hiding / encapsulation



  • single responsibility



  • separation of concerns



  • KISS (Keep it simple (and) stupid.)



  • DRY (Don't repeat yourself.)



  • "Tell! Don't ask."



  • Law of demeter ("Don't talk to strangers!")



  • favor polymorphism over branching


Interfaces, abstract classes, or inheritance support that principles and should be used as needed.

In that sense your code really does implement some of the OO principles.

But your entire program is in static context with is bad for a couple of reasons (especially for a "training" program):

  • no polymorphism



  • no dependency injection



  • hard to UnitTest



If you would actually use objects instead of just static methods you could make that code simpler and easier to extend.

The typical example is the switch in your method processUserSelection:

the OOP approach to this is to define a common interface for the operations.

interface AtmOperation {
  void operateOn(AccountCard account);
}


The we have a separate class for each operation (mot likely in its own class)

class WithdrawOperation implements  AtmOperation {
  @Override
  void operateOn(AccountCard account){
  }
}

class DepositOperation implements  AtmOperation {
  @Override
  void operateOn(AccountCard account){
  }
}

class BalanceOperation implements  AtmOperation {
  @Override
  void operateOn(AccountCard account){
  }
}


Some of that operations need resources from the ATM:

class BalanceOperation implements  AtmOperation {
  /* pretend we have a class AtmConsole with methods .redraw() and .println() */
  private final AtmConsole console;
  public BalanceOperation( AtmConsole console){
     this.console = console;
  }
  @Override
  void operateOn(AccountCard account){
     console.redraw();
    Locale currentLocale = Locale.US;
    int amount = account.getBalance();
    NumberFormat currencyFormatter = NumberFormat.getCurrencyInstance(currentLocale);
    console.println("Your current balance is now " + currencyFormatter.format(amount));
  }
}

class InputErrorOperation  implements  AtmOperation {
  private final AtmConsole console;
  public BalanceOperation( AtmConsole console){
     this.console = console;
  }
  @Override
  void operateOn(AccountCard account){
    console.redraw();
    console.println("Unknown operation please try again");
  }
}


There are different ways to access this implementations. The easiest way is to put them in a Map when you initialize the ATM:

// ...
private final Map operations = new HashMap<>();
private final AtmConsole console = new AtmConsole();
private final AtmOperation invalidInputOperation = new InvalidInputOperation(console);
public static void start() {
    operations.put("w",new WithdrawOperation(console));
    operations.put("d",new DepositOperation(console));
    operations.put("s",new BalanceOperation(console));
    operations.put("e",new ReturnCardOperation(console));
    atmIsActivated = true;
// ...


then your method processUserSelection would change to:

private static void processUserSelection(AccountCard account) {
    String userInput = Helper.getUserInput();    
    AtmOperation operation =   operations.getOrDefault(userInput.toLowerCase(),invalidInputOperation);
    operation.operateOn(account);
}


benefits are:

  • configurations, definition and usage of the operations are clearly separated.



  • you can change the operations without changing the place(s) where they are used.



  • you can put each operation in its own class file



  • you can easily write UnitTests for each operation to verify and document their behavior.




I assume the AtmConsole is the console of the Atm where all the transactional information and main menu is displayed. How would the AtmConsole look like?

Im my world there could be different console types. Therefore I had an AtmConsole interface:

interface AtmConsole{
   void redraw();
   void println(String message);
    /* most likely it should also handle the input
       Because it is the User Interface: */ 
   String aqireInput();
}


And This would be my implementation:

```
class AtmConsoleCommandLine implements AtmConsole {
private final Scanner input;
public AtmConsole(Scanner input){
th

Code Snippets

interface AtmOperation {
  void operateOn(AccountCard account);
}
class WithdrawOperation implements  AtmOperation {
  @Override
  void operateOn(AccountCard account){
  }
}

class DepositOperation implements  AtmOperation {
  @Override
  void operateOn(AccountCard account){
  }
}

class BalanceOperation implements  AtmOperation {
  @Override
  void operateOn(AccountCard account){
  }
}
class BalanceOperation implements  AtmOperation {
  /* pretend we have a class AtmConsole with methods .redraw() and .println() */
  private final AtmConsole console;
  public BalanceOperation( AtmConsole console){
     this.console = console;
  }
  @Override
  void operateOn(AccountCard account){
     console.redraw();
    Locale currentLocale = Locale.US;
    int amount = account.getBalance();
    NumberFormat currencyFormatter = NumberFormat.getCurrencyInstance(currentLocale);
    console.println("Your current balance is now " + currencyFormatter.format(amount));
  }
}

class InputErrorOperation  implements  AtmOperation {
  private final AtmConsole console;
  public BalanceOperation( AtmConsole console){
     this.console = console;
  }
  @Override
  void operateOn(AccountCard account){
    console.redraw();
    console.println("Unknown operation please try again");
  }
}
// ...
private final Map<String,AtmOperation> operations = new HashMap<>();
private final AtmConsole console = new AtmConsole();
private final AtmOperation invalidInputOperation = new InvalidInputOperation(console);
public static void start() {
    operations.put("w",new WithdrawOperation(console));
    operations.put("d",new DepositOperation(console));
    operations.put("s",new BalanceOperation(console));
    operations.put("e",new ReturnCardOperation(console));
    atmIsActivated = true;
// ...
private static void processUserSelection(AccountCard account) {
    String userInput = Helper.getUserInput();    
    AtmOperation operation =   operations.getOrDefault(userInput.toLowerCase(),invalidInputOperation);
    operation.operateOn(account);
}

Context

StackExchange Code Review Q#162667, answer score: 3

Revisions (0)

No revisions yet.