3
\$\begingroup\$

This is a follow up (improved) post to this. I tried to consider the points polymorphism, Single-Responsibility-Principle, separation of concerns as much as possible. (See the complete code on GitHub.)

I consider myself a beginner in Java. Therefore I want to improve and learn about Java 8 and OOP as much as I can. I'd like feedback on performance, clean code, and folder structure

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, e.g. negative withdrawal/deposit amount, exceeding the balance, wrong password, etc.

Main.java

import controller.Atm;

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

Atm.java

package controller;

import controller.atmOperations.*;
import model.BankCard;
import model.FileParameters;
import model.TransactionOptions;
import model.exceptions.*;
import java.util.*;

public class Atm {

    final static Map<String, AtmOperation> operation = new HashMap<>();
    final static AtmUserInterface ui = new AtmCommandLine();
    static SaveCard saveCard;

    public static void start() {
        while (true) {
            ui.redraw();
            System.out.println("Press ENTER to insert card");
            Scanner scanner = new Scanner(System.in);
            if ((scanner.nextLine() != null)) {
                BankCard account = new LoadCard(ui).start();
                initializeMethods(account);
                transactionalManipulationOf(account);
            }
        }
    }

    private static void initializeMethods(BankCard acc) {
        saveCard = new SaveCard(ui, acc);
        operation.put(TransactionOptions.DEPOSIT.getCode(), new DepositOperation(ui, acc));
        operation.put(TransactionOptions.WITHDRAW.getCode(), new WithdrawOperation(ui, acc));
        operation.put(TransactionOptions.SHOW_BALANCE.getCode(), new ShowBalanceOperation(ui, acc));
        operation.put(TransactionOptions.EXIT.getCode(), new ExitAtmOperation(ui, acc));
    }

    private static void transactionalManipulationOf(BankCard acc) {
        ui.redraw();
        do {
            ui.showMainMenu();
        } while (activateProcessingUserSelection(acc));
    }

    private static boolean activateProcessingUserSelection(BankCard acc) {
        return operateOn(ui.getUserInput());
    }

    private static boolean operateOn(String userInput) {
        try {
            switch (operation.get(userInput).start()) {
                case SHOW_BALANCE:
                    operation.get(TransactionOptions.SHOW_BALANCE.getCode()).start();
                case OK:
                case FAILED:
                case CANCELED:
                    return true;
                case RETURN_CARD:
                    operation.get(TransactionOptions.EXIT.getCode()).start();
                    return false;
                case KILL_CARD:
                    saveCard.start(FileParameters.IRRECOGNIZABLE_PIN.getValue());
                    return false;
                case SAVE_TRANSACTIONAL_CHANGES:
                    saveCard.start();
                    return false;
                default:
                    try {
                        throw new UnknownTransactionOperation("Unknown operation please try again.");
                    } catch (UnknownTransactionOperation unknownTransactionOperation) {
                        ui.printErrorMessage(unknownTransactionOperation);
                    }
                    return true;
            }
        } catch (NullPointerException e) {
            try {
                throw new UnknownTransactionOperation("Unknown operation please try again");
            } catch (UnknownTransactionOperation unknownTransactionOperation) {
                ui.printErrorMessage(unknownTransactionOperation);
                return true;
            }
        }
    }
}

AccountCard.java

package model;

import model.exceptions.*;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.Map;

public class AccountCard implements BankCard{
    String pin = null;
    int amount = 0;
    DateFormat dateFormat = new SimpleDateFormat(FileParameters.DATE_PATTERN.getValue());
    Map<String, Integer> transferHistory = new DefaultLinkedHashMap<>(SettingParameters.DEFAULT_VALUE_TRANSFER_HISTORY.getValue());
    boolean isCardInserted = false;

    public AccountCard(String pin, int amount, String withdrawDate, int timesWithDraw) {
        this.pin = pin;
        this.amount = amount;
        this.transferHistory.put(withdrawDate, timesWithDraw);
        this.isCardInserted = true;
    }
    @Override
    public String getPin() {return this.pin;}
    @Override
    public boolean isPinCorrect(String pinInputByUser) {return this.pin.equals(pinInputByUser);}
    @Override
    public int getBalance() {
        return amount;
    }
    @Override
    public void withdrawAmount(int withdrawAmount) throws OverdrawBankTransfer, ZeroBankTransfer, ExceedLimitTransfer, WithdrawalNegativeBankTransfer {
        if (withdrawAmount > this.amount) throw new OverdrawBankTransfer("Withdrawal exceed balance.");
        if (withdrawAmount < 0) throw new WithdrawalNegativeBankTransfer("Withdrawal of negative mount");
        if (withdrawAmount == 0) throw new ZeroBankTransfer("Withdrawing of zero amount is forbidden.");
        Date now = new Date();
        if (isTransferExceedLimit(now))throw new ExceedLimitTransfer("Limit of numbers of withdrawals exceeded");
        this.amount -= withdrawAmount;
        logBankTransfer();
    }
    @Override
    public void depositAmount(int depositAmount) throws DepositNegativeBankTransfer, ZeroBankTransfer {
        if (depositAmount < 0) throw new DepositNegativeBankTransfer("Invalid operation. Deposit amount is negative");
        if (depositAmount == 0) throw new ZeroBankTransfer("Depositing of zero amount is forbidden.");
        this.amount += depositAmount;
    }
    @Override
    public String getWithdrawDate() {
        String withdrawDate = "";
        for(Map.Entry<String, Integer> entry : transferHistory.entrySet()) {
            withdrawDate = entry.getKey();
        }
        return withdrawDate;
    }
    @Override
    public int getTimesWithDraw() {
        int timesWithDraw = 0;
        for (Map.Entry<String, Integer> entry : transferHistory.entrySet()) {
            timesWithDraw = entry.getValue();
        }
        return timesWithDraw;
    }
    private boolean isTransferExceedLimit(Date date) {
        String now = dateFormat.format(date);
        return transferHistory.get(now) > 3;
    }
    private void logBankTransfer() {
        Date date = new Date();
        String now = dateFormat.format(date);
        int timesWithDrawCurrent = transferHistory.get(now);
        transferHistory.put(now, ++timesWithDrawCurrent);
    }
}

AtmCommandLine.java

package controller;

import controller.helper.Helper;
import model.*;

import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;

public class AtmCommandLine implements AtmUserInterface{

    public AtmCommandLine() {};
    @Override
    public void redraw() {
        final String ANSI_CLS = "\u001b[2J";
        final String ANSI_HOME = "\u001b[H";
        System.out.print(ANSI_CLS + ANSI_HOME);
        System.out.flush();
        System.out.println("********* ATM *********");
    }

    @Override
    public String getUserInput() {
        Scanner scanner = new Scanner(System.in);
        return scanner.nextLine();
    }

    @Override
    public int getIntegerUserInput(String userInput) {
        while(Helper.inputIsNotInteger(userInput)) {
            System.out.println("Please type in a valid number (Integer)");
            userInput = getUserInput();
        }
        return Integer.parseInt(userInput);
    }

    @Override
    public void printErrorMessage(Exception e) {
        System.out.println("==========ERROR==========");
        System.out.println(e.getMessage());
        System.out.println("==========================");
    }

    @Override
    public PinRequestResult requestPin(BankCard account) {
        System.out.println("Please enter your PIN to execute transaction");
        int trials = 0;
        while (trials < SettingParameters.MAX_PIN_INPUT_COUNT.getValue()) {
            String userInput = getUserInput();
            if (account.isPinCorrect(userInput)) return PinRequestResult.VALID;
            if (userInput.equals(TransactionOptions.CANCEL.getCode())) return PinRequestResult.CANCELED;
            printWrongPinWarningMessage(++trials);
        }
        return PinRequestResult.IRREVOCABLE_INVALID;
    }

    @Override
    public void printWrongPinWarningMessage(int trials) {
        System.out.print("Incorrect PIN. ");
        Map<Integer, String> warningMessage = new HashMap<Integer, String>(){
            {
                put(1, "Please try again.");
                put(2, "You typed in your pin wrong twice already. This is your last try.");
                put(SettingParameters.MAX_PIN_INPUT_COUNT.getValue(), "You typed in the pin wrong trice. Transaction aborted. \n Your card is now invalid.");
            }
        };
        System.out.print(warningMessage.get(trials));
        System.out.println(" Or press c to cancel");
    }

    @Override
    public void showMainMenu() {
        System.out.println("=======Main-Menu=======");
        System.out.println("What do you want to do?");
        System.out.println("Withdrawal \t\t(w)");
        System.out.println("Deposit \t\t(d)");
        System.out.println("Show Account \t(s)");
        System.out.println("Exit \t\t\t(e)");
        System.out.println("________________________");
    }
}
\$\endgroup\$
1
\$\begingroup\$

What I'm missing the most is that class which performs the actual requests? For instance, the pinValidation is performed in the AccountCard type - this is imo mixing of responsibilities ("holding state" / "beeing a DTO" and "validating pins"). I'm also pretty sure, the actual pinCode of a card will never be transported to an ATM. I'd suggest write something like a AtmRequestService?

Not sure about the Atm class. It's more like a controller class, as in the MVC pattern. It takes user input and tells the ui what it has to show and does start transactions. The actual logic is within the Card type.

Why are you doing a mapping for AtmOperation and not working with the enum itself? Why not pass the enum in the operateOn method?

In the AtmCommandLine is actual business logic in the requestPin method. The "n trials before account is locked"-thing shouldn't be in the ui.

The ATM is actually a very good exercise for a traditional 2 or 3-tier application, but I can't see it here; I think your main problem is, you don't know where to put what code. Maybe a good exercise would be, to completely ignore the UI for a moment, and just focus on the business logic. And maybe get used to a few mantras: "When I want to exchange the UI, what do I have to change in my code?". And "Does that class do only one thing?". When you get that, put the UI on top of it. (It's not the usual thing to do, but I think it will help you). (And maybe write some unit tests, I'm just saying)

Smaller stuffs:

  • Everything in the Atm is static. I don't see any reason to do that?
  • Always make your members private. Like, always. Not always, but 99% of the time. Never make something accessible for convenience. Member vars are implementation, and implementation should be hidden.
  • The operateOn method is a bit confusing. First of all, it returns a boolean, but the caller doesn't know when it returns true or false. And actually, I have no idea what it should do, that really needs a better name.
  • catching NPE's: Uuuuh... Don't do that. When that error occurs in the operateOn method, you have no idea what the state of the Atm actually is, because that could have been thrown anywhere. The Atm should actually shut down for those uncatched exceptions, imo.
  • Throwing and catching Exceptions at the same time is quite a confusing pattern, too. You can call printErrorMessage and instantiate the exception directly in the parameter. IMO, exceptions shouldn't be used for controlling the flow. What I would have done is provide other methods like "isValidTransaction". If that returns false, don't even call the operateOn. So, your code flow doesn't actually allow unknown transactions. And if an unknown operation will be passed, it actually is an exception, because the application shouldn't be in that state, meaning: There's a bug in the code.
  • Also, always add the Exception suffix to Exceptions.
  • Working with Dates: You often instantiate a new Date(). This can backfire, when you have several statements for one transaction. It's a common pattern to have some sort of a Context class, which holds a date and call it 'transactionDate', so that within a transaction, you always work with the same date.
  • Working with Dates, 2: As long as you don't show a Date to a user, don't format it to a String. Working with dates is "not fun", and formatting Dates to String and work with the Strings later, makes it worse.
  • requestPin: Is badly named, too. You validate the pin. When I read requestPin, I think of "forgot pin, send me a new one, please".

Hope that helps,...

\$\endgroup\$
  • \$\begingroup\$ "Everything in the Atm is static. I don't see any reason to do that?" -> because there is only one ATM? The software should run on an ATM. Is this explanation valid? \$\endgroup\$ – thadeuszlay May 17 '17 at 12:49
  • \$\begingroup\$ "exceptions shouldn't be used for controlling the flow." -> How would you handle Exceptions, e.g. user deposits negative amount of money, user withdraws more money than he got on his account, etc. then? IMO, the ATM shouldn't shut down when one of these exceptions are thrown, right? Instead it should continue to run and ask the user to type in a valid input. \$\endgroup\$ – thadeuszlay May 17 '17 at 12:56
  • 1
    \$\begingroup\$ Satic: It's valid yes, but keep in mind, that there will be a lot of OO priniples which you can't implement: No inheritance, no Factory, no Facade, no Decorators and so on. It's a very rare thing, those 'static-only' types. \$\endgroup\$ – slowy May 17 '17 at 13:02
  • 1
    \$\begingroup\$ Exceptions: Use methods which return true/false or use a separate type which performs the validation. Anyway, you will have asynchronous manipulation of the data (two bank cards), the actual transaction will be performed somewhere central and will give you a corresponding result, e.g. a "not enough money"-error. But this error shouldn't be handled as exception, but as separate use case. An exception indicates that something has gone wrong and the transaction could not be performed because of unknown reasons. And then the ATM should shutdown and restart itself, to avoid bad things happening. \$\endgroup\$ – slowy May 17 '17 at 13:09
  • 1
    \$\begingroup\$ Uhm, more something like: 1. Read user input. 2. Validate, if amount is a number. 2.A. (isNumber): Process the transaction. 2.B. (isNotANumber): Give user the message, start over. Then you will have a lot of if/elses for all the validations, but there's patterns to tidy that up, see martinfowler.com/articles/replaceThrowWithNotification.html, that might be an approach you want to look into. (Also, quote: "if a failure is expected behavior, then you shouldn't be using exceptions", which verifies my point :P) \$\endgroup\$ – slowy May 17 '17 at 13:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.