In August 2008 Robert C Martin released a new book, Clean Code. It’s a well written practical book about how to write readable easy to maintain code. Since starting to read it I have also brought the concepts up during the code reviews we run every Friday.
Last Friday we discussed a compareTo method I had been working on during the morning. The aim was to turn a fairly simple but not that readable algorithm into a highly readable chain of functions. We used a few rules discussed in Clean Codes chapter about functions:
- Do one thing
- Don’t repeat yourself
- Use descriptive names
- One level of abstraction per function
Lets have a look at compareTo before the code review started:
|
|
public int compareTo(final Object _other) throws ClassCastException, NullPointerException {
final ExampleClass otherExampleClass = (ExampleClass) _other;
if(equals(otherExampleClass)) {
return 0;
}
if(getCreatedDate() == null) {
if(otherExampleClass.getCreatedDate() == null) {
return 0;
}
return -1;
}
if(otherExampleClass.getCreatedDate() == null) {
return 1;
}
return otherExampleClass.getCreatedDate().compareTo(getCreatedDate());
} |
There are a number of problems with the code above. One of my colleagues put it really well “I have to think whilst reading it, and that slows me down”.
Lets look at the problems:
- The function does loads of different things
- It casts other to an ExampleClass
- It checks if otherExampleClass is equal to this
- It check if this’s createdDate is null
- It checks if otherExampleClass’s createdDate is null
- It compares otherExampleClass’s createdDate with this’s createdDate
- It repeats the null check on otherExampleClass’s createdDate
- The names in the method does not read very well, it’s not clear when reading the function form top to bottom what is going on.
- The nested if statement create two levels of abstraction.
But even though the method is not following the rules set out above it is short and not to hard to understand. Is it not just nitpicking to break it apart. I was thinking so when we started, and so did my colleagues. But after some discussion we decided that it would still make a great exercise.
First of we had to break out all but one thing from compareTo and put it some were else. A simple extract method refactoring. The responsibility left to compare to was the cast, since it has to be preformed before everything else.
|
|
public int compareTo(Object _other) throws ClassCastException, NullPointerException {
final ExampleClass otherExampleClass = (ExampleClass) _other;
return compareMeTo(otherExampleClass);
} |
Now compareTo only does one thing, on one level of abstraction. It hands all other things to the next level.
The new function will now have loads of thing to do so we need to preform the same task there.
|
|
private int compareMeTo(final ExampleClass _otherExampleClass) {
if(equals(_otherExampleClass)) {
return 0;
}
return compareCreatedDates(_otherExampleClass);
} |
This is pretty much it. we continued to refactor, using extract method, until every function created was easy to read and only did one thing. The end result is below.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 |
public int compareTo(final Object _other) throws ClassCastException, NullPointerException {
final ExampleClass otherExampleClass= (ExampleClass) _other;
return compareThisTo( otherExampleClass);
}
private int compareThisTo(final ExampleClass _otherExampleClass) {
if(equals(_otherExampleClass)) {
return 0;
}
return compareCreatedDates(_otherExampleClass);
}
private int compareCreatedDates(final ExampleClass _otherExampleClass) {
if(isThisAndOthersCreatedDateNull(_otherExampleClass)) {
return 0;
}
if (isThisCreatedDateNull()) {
return 1;
}
if (isOthersCreatedDateNull(_otherExampleClass)) {
return 1;
}
return compareCreatedDatesThatAreNotNull(_otherExampleClass);
}
private boolean isThisAndOthersCreatedDateNull(final ExampleClass _otherExampleClass) {
return isThisCreatedDateNull() && isOthersCreatedDateNull(_otherExampleClass);
}
private boolean isThisCreatedDateNull() {
return isOthersCreatedDateNull(this);
}
private boolean isOthersCreatedDateNull(ExampleClass _otherExampleClass) {
return _otherExampleClass.getCreatedDate() == null;
}
private int compareCreatedDatesThatAreNotNull(ExampleClass _otherExampleClass) {
return _otherExampleClass.getCreatedDate().compareTo(getCreatedDate());
} |
Whilst doing this work we also discussed, with regards to the Don’t repeat yourself rule, if the methods isThisCreatedDateNull and isOthersCreatedDateNull didn’t do the same thing. Interestingly enough our IDE took care of that for us during the refactoring and replaced the repeating code that we then had in isThisCreatedDateNull.
This is a short example done with a trivial piece of code. The benefits are that when I need to check how the compareTo function works for ExampleClass the it’s an easy read and I’m not forced to decipher the content. It’s there in plain English. Imagine then if we use the same technique on a method that is 50 lines long, or 100. Sure, we will create a lot of methods but they will be easy to read and we can rest assured that they only do what it says on the label. And the last thing I want to find in my code base is surprises.
The exercise here uses the advices given in only one chapter of Clean Code. The book contains 17 chapters. You are free to use this exercise to improve the quality of your code but please don’t stop there. Get the book, read it and practise it. It is excellent advice that all programmers with self dignity and a sense of professionalism should practise. And it is as well written as the code it contains.
This article have also been posted at Javalobby and there is a discussion there on how to improve it.
Update 18th November 2011: This article now has a follow up.
In August 2008 Robert C Martin released a new book, Clean Code. It’s a well written practical book about how to write readable easy to maintain code. Since starting to...
