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.


Dependency Inversion Principle... Say what???

Dependency Inversion Principle

noun [C] / dɪˈpen.dən.si ɪnˈvɜː.ʒən prɪn.sə.pəl /

  • "A principle in OO-programming which states that high-level modules should not depend directly on low-level modules. Instead, both should depend upon abstractions. Abstractions should not depend upon details. Details should depend upon abstractions."

Well, that clears it up... errm... Not really! I mean, I can read. I understand the words. I am fairly bright. I even think that I 'kinda' know what it means. But it is still kind of vague...
... And that is exactly what it should be.... Vague. That's what a principle is about. It's a guide line, a moral rule, a code of conduct, a fundamental source, a basis (I think you get the point). Nothing more, nothing less!

Principles are just like politicians, they say a lot of stuff but they never provide a real solution. For example, in my hometown the local authorities enforce a policy called "The polluter pays". Period. It doesn't say what pollution exactly is. It doesn't state how it should be paid. Should it be paid in money? And if so, how much? When are you considered a polluter? It doesn't really give an answer, it simply says that when you are going to live here... be aware, we tend to let you, the polluter, pay for the damage you cause to the environment!

Applying this to the Dependency Inversion Principle, it just says, when writing our code we should try to stick to the fact that high(er) level objects should not depend on concrete implementations of low(er) level objects. It doesn't say how, why or when. It even does not say that we have to invert "everything" in the code that we write from now on. It just means that, this is our way of thinking!

Dependency Injection

But, what about Dependency Injection? Is that the same? Is it also vague? Nope... Dependency Injection is one of the ways of implementing the Dependency Inversion Principle. Let's try an analogy. In my spare time I am a musician. So a music analogy it is! Here goes...

Let's say that you recently discovered a kick-ass rock band called The Licks & Grooves All-Stars (insert band name of your liking here) which plays all of your favorite funky tunes.

A band usually consists of:

  • a band leader (that guy who tells other musicians in the band what to play, when to play certain grooves, licks, solo's and all kinds of stuff.
  • other musicians that follow the instructions of the band leader, in order to produce awesome groovy tunes as a band

The band leader is our high-level object. He is the one who is responsible for songs being played. However, he cannot do it alone. He needs the other musicans, to produce some cool music. No musicians, no music. In other words our band leader depends on the other musicians. So the other musicians are our low-level objects in this case.

Because of the busy touring schedule of the band, each member has at least one stand-in/substitute musician. If a member cannot make it to a gig, the substitute musician will take over and the gig can still go on instead of being cancelled. Every musician has his own unique style of playing. Let's take the drummer (yes, a drummer is also a musician!!!) and his substitute colleague for example. They could be implemented like this:

Low-level objects

The Drummer...

public class DaveGrohl {
    public void beatTheCrapOutOfTheBassDrum(){
        System.out.println("BOOOOOM");
    }

    public void whackTheSnareDrum(){
        System.out.println("TAK!!");
    }
}

... and his substitute

public class CharlieWatts {
    public void gentlyHitTheBassDrum(){
        System.out.println("dum");
    }

    public void strikeTheSnareDrum(){
        System.out.println("chik");
    }
}

As you can see, they basically do the same thing... They both can play a bass drum and a snare drum. They just do it in their own unique way!!!

High-level object

For the sake of keeping things simple, The Licks & Grooves All-Stars band consists of a band leader and a drummer only. Assume that our band leader wants to play "We Will Rock You!" (A famous song from a band called Queen). The code might look something like this:

public class BandLeader {
    private String drummer;

    public BandLeader(String drummer){
        this.drummer = drummer;
    }

    public void playWeWillRockYou() throws NoSongPlayedException {
        if (drummer.equals("Dave")){
            DaveGrohl daveGrohl = new DaveGrohl();
            daveGrohl.beatTheCrapOutOfTheBassDrum();
            daveGrohl.beatTheCrapOutOfTheBassDrum();
            daveGrohl.whackTheSnareDrum();
        } else if (drummer.equals("Charlie")){
            CharlieWatts charlieWatts = new CharlieWatts();
            charlieWatts.gentlyHitTheBassDrum();
            charlieWatts.gentlyHitTheBassDrum();
            charlieWatts.strikeTheSnareDrum();
        } else {
            throw new NoSongPlayedException("No drummer available!!!");
        }
    }
}

If we want to play "We Will Rock You" with "Dave Grohl" on drums, this would be our client code:

public class GigApp {
    public static void main(String[] args) throws NoSongPlayedException {
        BandLeader bandLeader = new BandLeader("Dave");
        bandLeader.playWeWillRockYou();
    }
}

The console would show:
BOOOOOM
BOOOOOM
TAK!!

If we pass "Charlie" into the constructor of our Band Leader, the output would be:
dum
dum
chik

Excellent!!! Mission acomplished! Our band leader can handle two different drummers and still play the same song. And we didn't even use Dependency Injection!!!

It works, doesn't it?

Yes it works. But, There is only one problem though: Not only does the band leader has to know what to play (Two bass drum hits followed by a hit on the snare drum). He also needs to know how he has to address each drummer to make them play that sequence of notes. And on top of that, he also needs to know how to create/construct each drummer.

So he is essentially stuck to these two drummers. If we want to add more drummers to the band then we would be in really big trouble! Each new drummer added to the band means an extra else-if clause in the BandLeader.playWeWillRockYou method.

And to make things worse, what if the sequence of notes changes and the band leader wants to add an extra hit to the snare drum when playing We Will Rock You? Every if clause must be altered then!!! Maintenance hell will surely follow now, if the band and/or the song keeps changing...

To solve these problems, we are going to invert the control regarding object creation. You can invert lots a things as a programmer like the flow, interfaces and object creation (This is called Inversion of Control). Nowadays, the term Inversion of Control is considered the same as Dependency Injection. But in reality IoC is a little bit broader. Inversion of control with regard to object creation is called Dependency Injection! That sounds cool, so let's see how a little bit of Dependency Injection works out for the band:

High-level object (in the case of DI)

We want two things solved:

  • The band leader should only have knowledge of what.to play (Two bass drums followed by a snare drum). He should not be bothered with knowing how to address each drummer in order to let them play what he wants
  • We don't want the band leader to be responsible for creating/constructing each drummer. (See above: "He should not be bothered with knowing how to address each drummer..."

Looking at the requirements, the ideal implementation of our Band Leader is:

public class BandLeader {
    private Drummer drummer;

    public BandLeader(Drummer drummer){
        this.drummer = drummer;
    }

    public void playWeWillRockYou() throws NoSongPlayedException {
        if (drummer == null)
            throw new NoSongPlayedException("No drummer available!!!");

        drummer.playTheBassDrum();
        drummer.playTheBassDrum();
        drummer.playTheSnareDrum();
    }
}

What just happened here? We made an abstraction (a simplification) of Charlie and Dave. They are drummers, so we created a Drummer Interface.

public interface Drummer {
    void playTheBassDrum();
    void playTheSnareDrum(); 
}

The band leader depends on an interface now (which is good, in this case). He can now speak in one language to a drummer. The band leader can interact with any drummer, as long as the drummer object implements the Drummer interface.

Low-level object (in the case of DI)

public class DaveGrohl implements Drummer {
    @Override
    public void playTheBassDrum() {
        System.out.println("BOOOOOM");
    }

    @Override
    public void playTheSnareDrum() {
        System.out.println("TAK!!");
    }
}

... and

public class CharlieWatts implements Drummer{
    @Override
    public void playTheBassDrum() {
        System.out.println("dum");
    }

    @Override
    public void playTheSnareDrum() {
        System.out.println("chik");
    }
}

Our main client code looks quite similar. The only difference is that we now pass a "Drummer" into the Band leader instead of a String. And now for our band leader it doesn't matter how many drummers we add to the band. As long as a concrete implementation of a drummer knows how to behave like a drummer, everything is peachy.

Instead of Dave Grohl telling the band leader how he should be addressed. It is the Band leader (or better, the abstraction) saying to Dave how he should play the drums if he wants to be part of the band... Inversion of Control!

We can now truly inject a drummer into the band leader.

public class GigApp {
    public static void main(String[] args) throws NoSongPlayedException {
        BandLeader bandLeader = new BandLeader(new CharlieWatts());
        bandLeader.playWeWillRockYou();
    }
}

Is this really better?

Yes it actually is. Our band leader object is much cleaner. No messy if statements. When a new drummer is added to the line-up, no more changes to the band leader object is required.

If the structure of a song changes, it would mean a single modification. Without Dependency Injection we would have to change each if statement in this case.

A bonus would be, easier unit testing. We could now easily use a Mock or a 'fake' Drummer.

So... We should use Dependency Injection all the time now? Hm, no... Certainly not. It depends on the requirements and the likeliness of changes to come. It wouldn't make much sense to implement Dependency Injection if the band has now (and in the future) got only one drummer... The first version of the band leader would be an okay alternative in that case

It all depends...


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.