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.


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.