Clean Code - Functions

This is an exercise in how to apply the principles found in Robert C Martins new book Clean Code. We performed it a code review last week on functions I had written earlier that day. The aim was to turn a fairly simple, but not that readable. algorithm into highly readable functions. We used a few rules discussed in Clean Code's 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:

1. 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
2. It repeats the null check on otherExampleClass's createdDate
3. 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.
4. 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 too 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 all, we had to break out all but one thing from compareTo and put it somewhere else - a simple extract-method refactoring. The responsibility left to compareTo was the cast, since it has to be performed before everything else.

public int compareTo(final Object _other)throws ClassCastException, NullPointerException 
{
  final ExampleClass otherExampleClass= (ExampleClass) _other;
   return compareThisTo( 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 compareThisTo(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.

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(final ExampleClass _otherExampleClass) 
{
   return _otherExampleClass.getCreatedDate() == null;
}
private int compareCreatedDatesThatAreNotNull(final 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 benefit is 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 are surprises.

The exercise here uses the advice 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.