From conditional to maintainable code (part 2)

Refactoring for maintainable code

In my previous blog, in my search for maintainable code, I ended up with an initial piece of code which needs a bit of refactoring (see code below).

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;
    }
}

The code meets the requirement but it still could use some bit of tweaking in terms of readability and maintainability. What exactly is wrong with the code? For starters, the method "withdraw" is a bit long. This is mainly caused by duplication. I've got three if branches which basically do the same thing.

Also, the CEO of ATM Services could decide, at a certain point in time, to equip the ATM with 100 euro and 5 euro bills. This would mean that my method would be extended with 2 more if statements. This looks very ugly and besides that, the service clearly violates SOLID's Open/Close principle.

Lets get rid of the duplication first, by extracting the logic inside the if statement. My would look something like this:

Second take

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

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

        if(amount >= FIFTY_EURO.getValue()){
            amount = processWallet(amount, FIFTY_EURO);
        }

        if(amount >= TWENTY_EURO.getValue()){
            amount = processWallet(amount, TWENTY_EURO);
        }

        if(amount >= TEN_EURO.getValue()) {
            processWallet(amount, TEN_EURO);
        }

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

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

    private int processWallet(int amount, Denomination denomination) {
        int quantity = amount / denomination.getValue();
        if (quantity != 0) {
            wallet.put(denomination, quantity);
        }

        return amount - (quantity * denomination.getValue());
    }
}

This is already a big improvement. But it still looks very nasty. The duplication of the logic inside the if statements is resolved but it still got all those if statements. Before I go on to try to come up with a solution for this problem I want to refactor the check on "requestedAmount". Why? because the check makes use of a temporary variable. requestedAmount is initialised at the top of the method and used at the bottom of the method. Furthermore, the check itself could be extracted in a separate method. This would make it much more readable.

Maintainable code in action

Before I do that I wonder if there isn't an easier way of checking that the ATM can (or cannot) dispense a certain amount of money. Furthermore, the check is executed at the end of the method. This is due to the fact that the check depends on the "wallet" being filled during the method. I would rather like to see the check failing fast. In other words, I want to move the check to the top of the method to act as a guard clause. After a bit of thinking I came up with the following solution.

Third take

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

    public Map withdraw(int amount){
        validate(amount);

        if(amount >= FIFTY_EURO.getValue()){
            amount = processWallet(amount, FIFTY_EURO);
        }

        if(amount >= TWENTY_EURO.getValue()){
            amount = processWallet(amount, TWENTY_EURO);
        }

        if(amount >= TEN_EURO.getValue()) {
            processWallet(amount, TEN_EURO);
        }

        return wallet;

    }

    private void validate(int amount) {
        if(amount % Denomination.smallest() != 0) {
            throw new IllegalArgumentException("ATM cannot dispense the requested amount!");
        }
    }

    private int processWallet(int amount, Denomination denomination) {
        int quantity = amount / denomination.getValue();
        if (quantity != 0) {
            wallet.put(denomination, quantity);
        }
        return amount - (quantity * denomination.getValue());
    }
}

What did just happen? I deleted the original check, just because I can (and because I use git). I implemented a new method "validate". The method checks if an amount of money is divisible by the smallest denomination (10 in this case). If it isn't then the ATM cannot dispense the requested amount. And I added a call to the "validate" method at the top of the "withdraw" method. With this the guard clause is in place.

Interesting to mention. As you can see I also delegated the responsibility of coming up with the smallest denomination to the enum itself. I do not let the "validate" method iterate over the enum values in order to come up with the smallest denomination. I just simply ask the enum itself "Enum what is your smallest value?". It improves the maintainability and more important, it prevents feature envy.

And? Is the code more maintainable?

Looking at the code I come to the conclusion that I am in a much better position than that I was at the beginning of the this post. The code is much cleaner and it is more readable and maintainable than that it was. But still I am not happy. I am still stuck with the nasty if statements. And I still have to find a way to deal with a variable configuration of denominations for the ATM.

There are several ways of dealing with the mentioned problems. In the next (and last) blog post of this series I will tackle the if's and the variable configuration of denominations by using the "Chain of responsibility" design pattern. We will see if all this refactoring makes the code more maintainable.