Avoiding FORs – Anti-If Campaign
Have you ever wondered how FORs impact your code? How they are limiting your design and more important how they are transforming your code into an amount of lines without any human meaning?
In this post we are going to see how to transform a simple example of a for (provided by Francesco Cirillio – anti-if campaign), to something more readable and well designed.
So let’s start with original code using FOR:
public class Department { private List<Resource> resources = new ArrayList<Resource>(); public void addResource(Resource resource) { this.resources.add(resource); } public void printSlips() { for (Resource resource : resources) { if(resource.lastContract().deadline().after(new Date())) { System.out.println(resource.name()); System.out.println(resource.salary()); } } } }
See printSlips method. So simple method, only 10 lines counting white lines, but violating one of the most important rule, this method is doing more than one thing inside it mixing different level of abstractions.
As Robert C. Martin note in his book ‘Functions Should Do One Thing. They Should Do Well. They Should do it only […]. If a function does only steps that are one level below the stated name of the function, then the function is doing one thing […].’.
So with the given definition of how a method should look, let’s recap with previous method, and see how many things are doing
printSlips method? Concretely four.
public void printSlips() { for (Resource resource : resources) { #Cycle if(resource.lastContract().deadline().after(new Date())) { #Selection #Media System.out.println(resource.name()); #Content System.out.println(resource.salary()); } } }
The method is cycling, selecting resources, accessing to content, and accessing to media. See that each of them belongs to different level of abstraction, printing to console should be in different level of checking if a resource has not expired its contract yet.
Let’s see the solution proposed by Francesco.
The first thing to do is splitting main functions into three classes and two interfaces, one for iterating resources, another one to choose whatever a resource has not expired yet, and another one for printing a resource. With this approach, we are creating a solution which is designed to be extended, and improving readability too.
And now it is time for the code:
Predicate interface will be used for implementing if a resource meets an implemented condition.
public interface Predicate { boolean is(Resource each); }
For example in our case, implementation of interface will look like:
public class InForcePredicate implements Predicate { public boolean is(Resource each) { return each.lastContract().deadline().after(new Date()); } }
We have move conditional to InForcePredicate class. Note that if we want to create a class that checks if contract is expired we would create a new class implementing Predicate with something like return each.lastContract().deadline().before(new Date());
Block interface will be the next interface which will implement the access to media. In this case to console:
public interface Block { void evaluate(Resource resource); }
And its implementation:
public class PrintSlip implements Block { public void evaluate(Resource resource) { System.out.println(resource.name()); System.out.println(resource.salary()); } }
Again note that changing where information is sent (console, file, network, …) it is a simply matter of implementing Block interface.
And last class is the one which contains an iterator over resources, and also provides methods to call each interface created previously:
public class ResourceOrderedCollection { private Collection<Resource> resources = new ArrayList<Resource>(); public ResourceOrderedCollection() { super(); } public ResourceOrderedCollection(Collection<Resource> resources) { this.resources = resources; } public void add(Resource resource) { this.resources.add(resource); } public void forEachDo(Block block) { Iterator<Resource> iterator = resources.iterator(); while(iterator.hasNext()) { block.evaluate(iterator.next()); } } public ResourceOrderedCollection select(Predicate predicate) { ResourceOrderedCollection resourceOrderedCollection = new ResourceOrderedCollection(); Iterator<Resource> iterator = resources.iterator(); while(iterator.hasNext()) { Resource resource = iterator.next(); if(predicate.is(resource)) { resourceOrderedCollection.add(resource); } } return resourceOrderedCollection; } }
See next three important points:
- First one is that constructor receives a list of resources.
- Second one is that select method receives a predicate which is executed into the iterator to know if resource is choosable to be printed or not. Finally returning a new instance of ResourceOrderedCollection with resources without an expired contract.
- Third one forEachDo method receives a Block interface which is called by every element of resources list.
And finally modified Department class using previous developed classes:
public class Department { private List<Resource> resources = new ArrayList<Resource>(); public void addResource(Resource resource) { this.resources.add(resource); } public void printSlips() { new ResourceOrderedCollection(this.resources).select(new InForcePredicate()).forEachDo(new PrintSlip()); } }
Observe that now printSlips method contains a single readable line with the same level of abstraction.
Take notice that class and interface names are taken from Francesco example, but if I should do the same I would choose more representative names. Cirillo’s approach is good, but has some minor aspects to consider. For example it has the ‘ vertical problem‘: the InForcePredicate instance from Predicate interface uses five lines of source code to encapsulate a single statement.
We have explored two possible solutions of a problem, being the last one the proposed by Cirillio. Also there are many other possible and correct solutions to this problem, for example using Template Method Pattern, or mixing the use of Lambdaj with (or without) Closures (Lambdaj syntax can be a bit confusing). All of them have their pros and cons, but all of them makes your code readable and more important, all functions do one thing, they do well and they do it only.
As final notes of this post, JDK 8 will provide support for closures natively, and also will provide many features that now are provided by Lambdaj. Meanwhile JDK 8 is not stable (planned for mid-final 2013) or for your legacy code (from the point of view of JDK 8), Lambdaj is a really good fellow traveler.
We keep learning.
Reference: Avoiding FORs – Anti-If Campaign from our JCG partner Alex Soto at the One Jar To Rule Them All blog.
I’d really want to see an example where replacing for’s with this structure is more reasonable. This is just unrealistic. That’s what you’ll get with closures, just pick another language or wait for it. I’d much rather pick a project with the first example than one with the “better” one ;-)
See comments on the original post. The idea is not bad, but this examples are really bad. 1 simple loop is replaced by several classes and methods with little to no benefit at the end.
This shows why a lot of people hate writing JAVA. Doing it the “right way” always leads to code bloat.
Always? No, this is just one advice on the “right way”, and one with a bad example.
I prefer the original example to your proposed improvements. Sure, the original could probably do with a little refactoring to separate the business logic from presentation (for example), but you shouldn’t need to jump through the hoops you’re jumping through here.
There might be a good point or two somewhere in this article, but it’s hopelessly obfuscated by that example.
Your PrintSlip::evaluate method does two of the initial four things you mentioned: accessing content and accessing media. I’m sure you can refactor it into three more classes and at least an interface or two.
Perfect example for “Over Engineering” :-)
You kidding me, right?
Glad someone else felt the same way I did… The original example was very clear, the “new and improved” version was so confusing. Ouch…
A complete class with just the meaning to wrap a foreach? Not really ..
Obviously in this case Java is not the right tool for the job.
Hi ! mhhh all previous comments may be right but the actual purpose of this POC (because this is just a POC) is to show that we can handle language block structure like for or if by choosing the right concept and with a better expressivity. OK in this sample the interest might be less visible but I’ve imagined reading the article that people would have understood what the author want to show. I have in mind code in projects where numerous for loop are imbricated without readability and without testability. What is clear is that java language is a… Read more »