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.


Unit Testing and Refactoring Legacy Code

In this video I will be unit testing and do some refactoring of some piece of legacy code (existing code without unit tests). The code is based upon the "Tire Pressure Kata" as described in "The Coding Dojo Handbook" by Emily Bache.

Goal

The goal of this unit testing/refactoring move is to ultimately remove the hard dependency in the Alarm class. As you will see, Alarm depends on Sensor (because Sensor is instantiated in the Alarm class). It would be better if we could somehow inject the Sensor class into the Alarm class, instead of creating the Sensor class in the Alarm class itself.

When refactoring you preferrably do not want to touch the production code when there are no unit tests available. The only exception to this rule of thumb is automated refactorings done by your IDE (like extract methods etc.)

Description of the Tire pressure Kata

The Alarm class is designed to monitor tire pressure and set an alarm if the pressure falls outside of the expected range. The Sensor class provided for the excercise fakes the behaviour of a real tire sensor from a racing car, providing random but realistic values. Write the unit tests for the Alarm class.

Always test in production...
Don't be a wuss, always test in production!!!

First step : Unit Testing

First I covered the Alarm class with unit tests before the actual refactoring takes place. Only automated refactorings are done while testing. Sensor class is instantiated inside the Alarm class, so mocking the Sensor is out of the question. To work around this problem, I created a "seem" and used inheritance in my Alarm test class.

Second step : Refactoring

Next up was the actual refactoring move, injecting Sensor into the Alarm class. The last step was to get rid of the "AlarmShould" subclass (subclass of Alarm) by means of a Mock object. While refactoring I also tried to take care of the readabillity of the code.

Note that at the end of the refactoring session, the signature of the constructor has changed. Any other class that depends on Alarm must also be changed.