Tomas has posted 4 posts at DZone. You can read more from them at their website. View Full User Profile

Clean Code - Functions

02.03.2009
| 11685 views |
  • submit to reddit

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.

Published at DZone with permission of its author, Tomas Malmsten.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

John Baker replied on Tue, 2010/05/11 - 6:25am

I like the before rather than the after code I see way many flaws in your coding. 1. Your valued object the Date Object should not do the logic. 2. All logic should be in the business logic class that gets these valued objects 3. Only pass in things that are needed rather than the context 4. Don't use lengthy variable names, its difficult to read 5. Don't spam functions unnecessarily when it can be done in few lines. 6. Only use if statements when comparing primitive values 7. Use polymorphism to avoid using if for states If you want to follow what Robert Martin rules:- Just do these only:- 1. Use long descriptive function names if its called once twice. 2. Use short descriptive function names if its called many times. 3. Avoid using many functions in a class, if you have too many functions, it means your class has way too many responsibilities, delegate it into another class to deal with it. (In real life, you don't have many drawers in a bedroom, only one or two) What Roberts says in the video , having 5000 lines of functions is a bad thing, he should have delegated those into several classes.

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.