No more static methods

Static methods

Tired of those static methods that make your unit test skills look bad? You want to get rid of those hard to mock static methods? You refuse to use PowerMock? Here's a simple way of replacing static methods with non-static methods without breaking your entire code base (or worse someone else's code base).

Example

Let's say we've got the following service class (PostService) which depends on a class with a static method (PostDao).

public class PostService { 
  
  public List getPostsOf(User user){ 
    if(user != null){ 
      return PostDao.findByUser(user); 
    } 
  
    throw new UserNotLoggedInException("User not logged in!!!"); 
  } 
}
public class PostDao { 
  
  public static List findByUser(User user){
    // Some very interesting code for retrieving posts of a given user 
    //.. 
    //.. 
    //.. 
  } 
}

The simplest way would obviously be: remove the keyword "static", watch all the code (that call this method) brake, fix it and off you go. That sounds easy. But what if you don't want to replace the references to the static method in a big-bang fashion, for whatever reason. How would you go about that? Here are the steps:

  • Wrap the static method in a non-static method
  • Replace the reference to the static method with the instance method

Wrapping the static method

This is the easiest part. Create an instance method, in the class with the static method, with the same signature and return type. Call the static method from within the instance method.

The class would look like this

public class PostDao { 
  
  public static List findByUser(User user){
    // Some very interesting code for retrieving posts of a given user 
    //.. 
    //.. 
    //.. 
  } 
  public List findBy(User user){ 
    return findByUser(user); 
  } 
}

Now you're all set to proceed with the second step.

Replacing the references to the static method

Wrapping the static method allows you to replace all the references whenever you want. This enables you to repair your code base at your own pace.

If you want to repair the PostService you should do the following:

  • Create a No-args constructor (for backwards compatibility)
  • Create a constructor which accepts a PostDao instance, for easier testing/mocking purposes
  • Replace the static method call with an instance method call

When all the work is done, the result would be as follows

public class PostService { 
  
  private PostDao postDao; 

  @Deprecated 
  public PostService(){ 
    this(new PostDao()); 
  } 

  public PostService(PostDao postDao) { 
    this.postDao = postDao; 
  } 

  public List getPostsOf(User user){ 
    if(user != null){ 
      return postDao.findBy(user); 
    } 

    throw new UserNotLoggedInException("User not logged in!!!"); 
  } 
}

The PostService will still work, because of the No-args constructor, and does not break any existing code referencing the PostService. The constructor which accepts the PostDao instance allows for easier testing/mocking. You can annotate the No-args constructor with the 'Deprecated' annotation. This shows the intent of removing the no-args constructor somewhere in a future release.

If there are no more references to the static PostDao method you can move the logic which resides in the static method into the instance method and delete the static method. Furthermore, if there are no more references to the no-args constructor in the PostService you can remove the deprecated constructor. This can all be done in a non-big-bang-kinda-fashion...

End result

public class PostDao { 
  public List findBy(User user){ 
    // Some very interesting code for retrieving posts of a given user 
    //.. 
    //.. 
    //.. 
  } 
}

Building for readability

Spot the difference

Readability of code should not only be pursued in production code. It is also very important to make your (unit)tests as readable as possible. One of my favourite patterns for enhancing readability is the builder pattern.

Take a look at the following code snippet of a unit test. As you will notice, the creation of the User object involves a lot of lines. Not also does it involve a lot of lines of code, it isn't quite readable as I would like it to be.

    ...

    @Test
    public void not_return_any_tweets_when_user_is_not_followed_by_logged_in_user() throws UserNotLoggedInException {
        tweetService = new TweetService(PIERCE, tweetDAO);

        User james = new User();
        james.setName("James Bond")
        james.addFollower(ROGER);
        james.addFollower(TIMOTHY);
        james.addFollower(DANIEL);
        james.addTweet(ABOUT_ACTORS);
        james.addTweet(ABOUT_MOVIES);

        assertThat(tweetService.getTweetsByUser(james).size(),is(0));
    }

    ...

Don't get me wrong, the code above is quite readable but if you take a look at the code below it reads a bit more nicer. It is almost like you are reading a normal sentence. That is what the builder pattern gives you. Fluent readable object creation.

    ...

    @Test
    public void not_return_any_tweets_when_user_is_not_followed_by_logged_in_user() throws UserNotLoggedInException {
        tweetService = new TweetService(PIERCE, tweetDAO);

        User james = aUser()
                        .named("James Bond") 
                        .followedBy(ROGER, TIMOTHY, DANIEL)
                        .withTweets(ABOUT_ACTORS, ABOUT_MOVIES)
                        .build();

        assertThat(tweetService.getTweetsByUser(james).size(),is(0));
    }

    ...

Note that the static import is used in order to call aUser() without the class name. Otherwise it would read Userbuilder.aUser(). The latter is also readable but the first one reads a bit more natural. (But that is nitpicking 2.0)

Getting it done

How does that pattern work, you might think. The basic idea is that while creating the object the the builder saves all the data and when all is ready the build() method is invoked and at that point the object is created.

In order to chain the methods together all the methods that collect data (e.g. named an followedBy) should return the Builder object itself.

Creating the builder contains three steps:

  • Creating the static method which returns a new Builder (opening method)
  • Creating the data collecting methods which return the builder (intermediate methods)
  • Creating the build method (terminating method)

Opening method

This static factory method (not to be confused with the Factory design pattern) is added pure for enhanced readability.
If we didn't have this static method the builder would has to be invoked something like this: new UserBuilder().followedBy(...). The intent is less clearer as opposed to using the static factory method.

    ...

    public static UserBuilder aUser() {
        return new UserBuilder();
    }

    ...

Intermediate methods

In order to chain the methods each intermediate method should return itself. Notice the usage of the varargs parameter. By using the varags we delegate the adding of followers one-by-one to the builder itself. This is also the case for the withTweets(...) method.

    ...

    public UserBuilder followedBy(User... followers) {
        this.followers = followers;
        return this;
    }

    ...

Terminating method

For actually creating the User object, the builder must have a terminating method. This method is responsible for creating the actual object needed by the client. In this method we see that every piece of data, collected by the intermediate methods, is transferred to the User object and finally the constructed user is returned to the client.

    ...

    public User build() {
        User user = new User();
        setNameFor(user);
        addTweetsTo(user);
        addFollowersTo(user);
        return user;
    }

    ...

The full builder

public class UserBuilder {
    private String name = "";
    private User[] followers = new User[]{};
    private Tweet[] tweets = new Tweet[]{};

    public static UserBuilder aUser() {
        return new UserBuilder();
    }

    public UserBuilder named(String name) {
        this.name = name;
        return this;
    }

    public UserBuilder followedBy(User... followers) {
        this.followers = followers;
        return this;
    }

    public UserBuilder withTweets(Tweet... tweets) {
        this.tweets = tweets;
        return this;
    }

    public User build() {
        User user = new User();
        setNameFor(user);
        addTweetsTo(user);
        addFollowersTo(user);
        return user;
    }

    private void setNameFor(User user) {
        user.setName(name);
    }

    private void addFollowersTo(User user) {
        for (User follower : followers){
            user.addFollower(follower);
        }
    }

    private void addTweetsTo(User user) {
        for (Tweet tweet : tweets){
            user.addTweet(tweet);
        }
    }
}

Conclusion

According to Wikipedia "The intention of the builder pattern is to find a solution to the telescoping constructor anti-pattern that occurs when the increase of object constructor parameter combination leads to an exponential list of constructors. Instead of using numerous constructors, the builder pattern uses another object, a builder, that receives each initialization parameter step by step and then returns the resulting constructed object at once.

I would like to add that the builder pattern can also be used for enhanced readabilty and easier creation/validation of objects.


From conditional to maintainable code (part 3)

The last mile

I've been refactoring the ATM Service class in my last 2 posts of this blog post series. Although the class is more maintainable and readable as it is, I am still not there where I want to be. Two more things are in serious need of improvement for my solution to be maintainable. The if statements somehow need to be removed and the dispenser configuration should be dynamically configurable. As mentioned in my last post I am going to try to achieve this using the "Chain of responsibility" design pattern.

Before

This is what we ended up with at the end of the previous post

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

    public Map<Denomination, Integer> 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());
    }
}

Chain of responsibilty

What does this design pattern helps us solve? Coupling the sender (object) of a request to its receivers should be avoided. And that's exactly the problem that this pattern solves. A characteristic of this pattern is that it should be possible that more than one receiver (object) can handle a request. Otherwise it wouldn't be a chain I guess.

The above implies that there should be at least two receiver objects. Examining the code above we see that the wallet object is processed multiple times in a certain order. So there we have our receiver objects.

Chain of responsibility ATM as a maintainable chain of responsibilities

Quick recap. For this pattern we need a sending object. The ATM service class. Check. A request. Adding money to the wallet object. Check. And more than one receiver object (which should have a chance of handling the request). The dispensers. Check. So, choosing the chain of responsibility pattern might turn out to be a good call.

Sending object

Looking at the code below, the processing of the three if statements have been removed and delegated to the "Dispenser" object. Or actually, three Dispenser objects.

The ATM checks if the requested amount can be dispensed. Then the service creates the chain of dispensers (50 EUR dispenser -> 20 EUR dispenser -> 10 EUR dispenser). The ATM Service holds only one reference to the first dispenser. The service basically "fires-and-forgets" the request into the Dispenser chain and let the chain handle the process of adding the requested amount of money to the wallet. And finally, when the dispenser chain has finished processing, the service returns the wallet object to the calling client.

public class ATMService {
    private Map<Denomination, Integer> wallet = new HashMap<>();
    private Dispenser dispenser;


    public Map<Denomination, Integer> withdraw(int amount){
        validate(amount);

        dispenser = Dispenser.create( new Dispenser(FIFTY_EURO),
                                      new Dispenser(TWENTY_EURO),
                                      new Dispenser(TEN_EURO)
                );

        dispenser.execute(amount, wallet);

        return wallet;

    }

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

}

Receiving object(s)

The Receiver object is responsible for the following things:

  • creation of the chain of dispensers (not for the creation of the dispensers itself!!)
  • holding a pointer to the next dispenser in the chain
  • adding the amount of money to the wallet in the right combination of denominations
  • passing the request to the next dispenser in the chain (if there is a next dispenser)

The wallet is passed on to the chain. The wallet visits every Dispenser defined in the chain. When the end of the chain is reached the processing of the wallet terminates and the ATM Service can return the manipulated wallet object to the client.

public class Dispenser {
    private Dispenser nextDispenser;
    private Denomination denomination;

    public Dispenser(Denomination denomination) {
        this.denomination = denomination;
    }

    public static Dispenser create(Dispenser... dispensers) {
        for (int i = 0; i < dispensers.length; i++) {
            if (i == dispensers.length - 1)
                return dispensers[0];

            dispensers[i].setNextDispenser(dispensers[i + 1]);
        }
        throw new IllegalStateException("Chain error!");
    }

    public void setNextDispenser(Dispenser nextDispenser) {
        this.nextDispenser = nextDispenser;
    }

    public void execute(int amount, Map<Denomination, Integer> wallet) {
        int remainder = addToWallet(amount, wallet);

        if(nextDispenser != null)
            nextDispenser.execute(remainder, wallet);
    }

    private int addToWallet(int amount, Map<Denomination, Integer> wallet) {
        if (numberOfDenominations(amount) != 0) {
            wallet.put(denomination, numberOfDenominations(amount));
        }

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

    private int numberOfDenominations(int amount) {
        return amount / denomination.getValue();
    }
}

The extra mile

"We are done", you might think. Nope... almost. We got rid of the three if statements but we still have this issue of not being able to dynamically set the available dispensers. The chain is created within the ATM service and will therefore always consist of the three dispensers added at compile-time.

Fortunately, this is a relatively small change. All we have to do is remove the creation of the dispenser part and create a constructor which receives a dispenser (chain) object;

Once we do that we end up with an ATM Service class that looks something like this:

public class ATMService {
    private Map<Denomination, Integer> wallet = new HashMap<>();
    private Dispenser dispenser;

    public ATMService(Dispenser dispenser){
        this.dispenser = dispenser;
    }

    public Map<Denomination, Integer> withdraw(int amount){
        validate(amount);
        dispenseToWallet(amount);
        return wallet;
    }

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

    private void dispenseToWallet(int amount) {
        dispenser.execute(amount, wallet);
    }
}

What did all this refactoring got me? A compact and well-balanced ATM Service class. A more than well readable class. A more evenly distribution of responsibilities. And a very maintainable class. If e.g. a new dispenser has to be added to the ATM it only requires a modification in the enum and the addition of a new dispenser in the chain. The ATM Service and the dispenser object itself never have to be modified for a change like this!!! It adheres to the Open/Close principle

Well... There is maybe a small change needed. The "validate" method needs to be changed. But that's because the implementation of the "validate" method isn't quite right. But that has got nothing to do with the design pattern itself. I will leave it up to you as a form of exercise.

ATM in action

Below an example of the ATM Service in action. The chain is created outside the ATM Service class. The Service does it's magic and a wallet is returned.

public class ATMClient {
    public static void main(String[] args) {
        Dispenser dispenser = Dispenser.create(
                new Dispenser(Denomination.FIFTY_EURO),
                new Dispenser(Denomination.TWENTY_EURO),
                new Dispenser(Denomination.TEN_EURO)
        );

        ATMService service = new ATMService(dispenser);
        Map<Denomination, Integer> wallet = service.withdraw(180);

        System.out.println("Wallet : " + wallet);
    }
}

Output:
"Wallet : {TWENTY_EURO=1, FIFTY_EURO=3, TEN_EURO=1}"

Going all out

I am finally done... This would be a really good time for me to stop. Really. I could stop right here, do a git-commit and be happy. But I still could do an extra thing or two for more readability. One of those things is creating a separate builder for the creation of the chain of dispensers. This seems like a bit of over-engineering. And maybe it actually is. But just for the fun of it, lets see what we end up with.

What I am trying to do is to move the responsibility of creating the chain of dispensers from the Dispenser class to another class. Here goes.

1. Remove the create method out of the Dispenser class.

public class Dispenser {
    private Dispenser nextDispenser;
    private Denomination denomination;

    public Dispenser(Denomination denomination) {
        this.denomination = denomination;
    }

    public void setNextDispenser(Dispenser nextDispenser) {
        this.nextDispenser = nextDispenser;
    }

    public void execute(int amount, Map wallet) {
        int remainder = addToWallet(amount, wallet);

        if(nextDispenser != null)
            nextDispenser.execute(remainder, wallet);
    }

    private int addToWallet(int amount, Map wallet) {
        if (numberOfDenominations(amount) != 0) {
            wallet.put(denomination, numberOfDenominations(amount));
        }

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

    private int numberOfDenominations(int amount) {
        return amount / denomination.getValue();
    }
} 

2. Create the Builder and put the create method, which we removed previously from the Dispenser class, into the Builder class.

public class DispenserBuilder {
    private Denomination[] denomination;

    public static DispenserBuilder aDispenserchain() {
        return new DispenserBuilder();
    }

    public DispenserBuilder consistsOf(Denomination ... denominations) {
        this.denomination = denominations;
        return this;
    }

    public Dispenser build() {
        return createChain(new Dispenser[denomination.length]);
    }

    private Dispenser createChain(Dispenser[] dispenser) {
        assemble(dispenser);
        return link(dispenser);
    }


    private void assemble(Dispenser[] dispenser) {
        for (int i = 0; i < denomination.length; i++) {
            dispenser[i] = new Dispenser(denomination[i]);
        }
    }

    private Dispenser link(Dispenser[] dispenser) {
        for (int i = 0; i < dispenser.length; i++) {
            if (i == dispenser.length - 1)
                return dispenser[0];

            dispenser[i].setNextDispenser(dispenser[i + 1]);
        }
        throw new IllegalStateException("Chain error!");
    }
}

Now we can create the chain with the builder.

Dispenser dispenser = aDispenserchain()
                        .consistsOf(FIFTY_EURO, TWENTY_EURO, TEN_EURO)
                        .build();

I will say it again. Use with caution because this could be labeled as over-engineering, syntactic sugar or whatever you would like to call it. It's really a matter of taste! But hey, it looks cool anyway.

The full solution can be cloned or downloaded at GitHub


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.


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.