From conditional to maintainable code (part 1)

What the IF??

Recently I was fiddling around in my spare time doing some experimenting on some kata stuff. I was trying to implement a simple representation of an automated teller machine (ATM) in several different ways. Besides exploring the different ways of solving this kata the goal was also to produce some readable and maintainable code. So, here we go. We got this.

Boundaries and requirements

I've got a vivid imagination and I like to visualise things. Now for this exercise, suppose that the CEO of ATM Services wants me to build an ATM API. The only thing that the API has got to do is to enable customers to withdraw money from their account via an ATM. And it is my job to come up with a working product.

The ATM service has one public method called "withdraw". The method takes an amount argument representing the amount of money a customer wants to withdraw from the ATM. The method returns a "wallet" object. The wallet is represented by a Map<Denomination, Integer>. By "Denomination" I mean the face value of a banknote (or a coin). The integer represents the quantity of a certain type of denomination.

The skeleton of the ATMService looks like this

public class ATMService {
    public Map withdraw(int amount){
        ...
    }
}

The only requirement is that ATM should dispense the combination of denominations that produces the least amount of denominations. E.g. if a customer wishes to withdraw 60 euros, the ATM should return one 50 euro bill and one 10 euro bill in the wallet instead of six 10 euro bills. The ATM only contains 50, 20 and 10 euro bills. A future requirement could be a variable configuration of available denominations. For the sake of the exercise the ATM always has an unlimited amount of bills to dispense.

Denominations are represented by an enum

public enum Denomination {
    FIFTY_EURO(50), TWENTY_EURO(20), TEN_EURO (10);

    private int value;

    Denomination(int value) {
        this.value = value;
    }

    public int getValue() {
        return value;
    }
}

First take

On the first take I wanted to produce the simplest implementation for the problem without any notion of producing readable and maintainable code.

public class ATMService {
    private Map wallet = new HashMap<>();

    public Map withdraw(int amount){
        if(amount >= FIFTY_EURO.getValue()){
            int quantity = amount / FIFTY_EURO.getValue();
            if (quantity != 0) {
                wallet.put(FIFTY_EURO, quantity);
            }

            amount = amount - (quantity * FIFTY_EURO.getValue());
        }

        if(amount >= TWENTY_EURO.getValue()){
            int quantity = amount / TWENTY_EURO.getValue();
            if (quantity != 0) {
                wallet.put(TWENTY_EURO, quantity);
            }

            amount = amount - (quantity * TWENTY_EURO.getValue());
        }

        if(amount >= TEN_EURO.getValue()) {
            int quantity = amount / TEN_EURO.getValue();
            if (quantity != 0) {
               wallet.put(TEN_EURO, quantity);
            }
        }

        return wallet;
    }
}

Maintainable?

Unit test

My unit tests are all in the green so I am happy. But looking at the code, I could always go for some more happiness. There are some major flaws with this implementation but for a first take it's pretty alright (for now). The requirement of dispensing the least amount of bills has been met. Check. We git-commit the code and push it into the CI/CD pipeline with the speed of light and get our "potentially shippable product" on the way and deployed before lunch.

After an hour the phone rings! It's the CEO of ATM Services. "There is an error in the ATM Service, fix it immediately!" he says. "Our customers are getting short-changed". "If a customer wants to withdraw 195 euro the ATM only returns 190 euro!" he then continues. I then realise that I forgot to implement some sort of check. Saying to the CEO that that part wasn't in the requirements in the first place isn't going to cut it. So I start fixing the problem right away and under pressure I come up with the following code:

public class ATMService {
    private Map wallet = new HashMap<>();

    public Map withdraw(int amount){
        int requestedAmount = amount;

        if(amount >= FIFTY_EURO.getValue()){
            int quantity = amount / FIFTY_EURO.getValue();
            if (quantity != 0) {
                wallet.put(FIFTY_EURO, quantity);
            }

            amount = amount - (quantity * FIFTY_EURO.getValue());
        }

        if(amount >= TWENTY_EURO.getValue()){
            int quantity = amount / TWENTY_EURO.getValue();
            if (quantity != 0) {
                wallet.put(TWENTY_EURO, quantity);
            }
            amount = amount - (quantity * TWENTY_EURO.getValue());
        }

        if(amount >= TEN_EURO.getValue()) {
           int quantity = amount / TEN_EURO.getValue();
           if (quantity != 0) {
               wallet.put(TEN_EURO, quantity);
            }
        }

        int total = 0;
        for(Map.Entry entry : wallet.entrySet() ){
            total = total + (entry.getValue() * entry.getKey().getValue());
        }

        if(requestedAmount > total){
            throw new IllegalArgumentException("Cannot dispense the amount!");
        }
        return wallet;
    }
}

My tests pass and the code is pushed again into production. Just in time! In this case we are talking about a relatively small adjustment in the code. But as small as the code is, it is not as maintainable and/or readable as I would like. And on top of that I didn't think about the (possible) future requirement when cranking out the code on my laptop in the first place. If the changes and bugs start piling up, it will be a mess in no time. Refactor time!

In the next part of this blog I will take a closer look at the produced code and examine it a little further. I will try to come up with a few improvements in order to come closer in fulfilling the requirements and goals.