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
| 11959 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

Guillaume Jeudy replied on Tue, 2009/02/03 - 10:53am

Tomas,

Nice article even though you have to be careful about not overdoing factoring into separate methods. I do agree that methods should be kept small and focused. However if you have trouble finding a good name for a method then it probably means its doing too much or not enough... Every method should pull it's weight.

Other than that, with Java 1.5 you can genericize int compareTo(Object o) method by specifying:

 implements Comparable<ExampleClass> and then you can write:

 int compareTo(ExampleClass o)

 saving yourself the burden of casting and at the same time getting compile-time type checking.

Tracy Nelson replied on Tue, 2009/02/03 - 11:10am

Just to pick a nit, I notice that a lot of people are misunderstanding the intent of the "start boolean methods with 'is'" guideline.  It's a good idea when you're interrogating an object for its intrinsic state, but the real intent is to render the method name as a predicate to make it more readable.  So you'll have isInitialized(), isReady(), isNew(), etc.  It starts to fall apart when you apply it blindly, though.  Which of the following is easier to read:

if(isThisCreatedDateNull())

or

if(thisCreatedDateIsNull())

 Interestingly, you do use this naming convention in

compareCreatedDatesThatAreNotNull()

 

Nishant Kumar replied on Tue, 2009/02/03 - 12:58pm

Actually in this case I would prefer the original unclean code.

Tomas Malmsten replied on Tue, 2009/02/03 - 2:49pm in response to: Guillaume Jeudy

I'm not sure where you mean that I use to short methods. Please give some examples on how you would change the code to make it better in this aspect.

The project in which I wrote this compareTo method is running on an old version of Oracles Application Server which is also running Oracle Forms applications. So unfortunatly I don't have the luxury of Java 5 Generics.

Tomas Malmsten replied on Tue, 2009/02/03 - 2:00pm in response to: Tracy Nelson

You are indeed correct. It's much easier to read if(thisCreatedDateIsNull()).

Gian Franco Casula replied on Tue, 2009/02/03 - 2:14pm

Did you consider 'extract constant' for -1, 0, 1, maybe calling it SMALLER, EQUAL, GREATER?

Tomas Malmsten replied on Tue, 2009/02/03 - 2:55pm in response to: Gian Franco Casula

It didn't occur to us when we did the exercise. It would however greatly improve readability.

Thanks

Mwanji Ezana replied on Tue, 2009/02/03 - 4:35pm

I understand "do one thing" differently. Isn't the single responsibility principle that a class or method should only have one reason to change? The only reason compareTo() has to change is if the way ExampleClass instances are compared changes, so it's not breaking that rule.

I think you'd want to get to a Composed Method ("one level of abstraction"), as you do in compareCreatedDates(). For me, the two methods above that are pointless. Integrating gianfranco's excellent idea (in real life, those constants would be in a non-instantiable class), here's my proposal:

 

public int compareTo(final Object _other) throws ClassCastException, NullPointerException {
final ExampleClass other = (ExampleClass) _other;
if (equals(other)) {
return EQUAL;
}
if (bothDatesAreNull(getCreatedDate(), other.getCreatedDate())) {
return EQUAL;
}
if (dateIsNull(getCreatedDate())) {
return SMALLER;
}
if (dateIsNull(other.getCreatedDate())) {
return BIGGER;
}
return compareBothDates(other);
}

private int compareBothDates(final ExampleClass other) {
return other.getCreatedDate().compareTo(getCreatedDate());
}

private boolean bothDatesAreNull(Date date1, Date date2) {
return dateIsNull(date1) && dateIsNull(date2);
}

private boolean dateIsNull(Date date) {
return date == null;
}

private static final int BIGGER = 1;
private static final int SMALLER = -1;
private static final int EQUAL = 0;

 Though I haven't handled it here, shouldn't you handle comparing your instance to null? Looking at it now, it's easy to see that there's no logic that is really central to this class: dateIsNull() and compareBothDates() could easily be extracted into a utility class for reuse elsewhere.

Pete Cox replied on Tue, 2009/02/03 - 7:33pm

Since Java 5, the Comparable interface has been generic. No need for a cast to ExampleClass:

public int compareTo(ExampleClass ec){...}

Tomas Malmsten replied on Wed, 2009/02/04 - 3:27am in response to: Mwanji Ezana

I like this sugestion, but I don't like the second if statement. It's to much to read to make it effortless. I think the function name needs to be changed but I'm not sure to what.

  if (bothDatesAreNull(getCreatedDate(), other.getCreatedDate())) {

With regards to hadeling null, the contract for comapreTo states that if this or other is null the method has to throw a null pointer.

Can unfortunatly no use generics since the method is written in Java 1.4. :-(

Michal Chmielarz replied on Wed, 2009/02/04 - 3:46am

Proposal of Mwanji is more readable than the result of refactoring in post. It seems to be simpler.

Peter Ufak replied on Wed, 2009/02/04 - 4:50am

By my opinion the original method is simpler to read and understand and does one thing: it is sequence of tests written by simple way. May be, I agree in most complex methods.

Thomas Mueller replied on Wed, 2009/02/04 - 4:54am

Do you know ravioli code? For me, it means "many small methods that don't do much except calling other methods". You should avoid that. Some methods in the 'end result' are ravioli code.

Gernot replied on Wed, 2009/02/04 - 6:51am

You can't be serious:

you consider this simple expression

if(getCreatedDate() == null)

less readable than

if(thisCreatedDateIsNull())

IMHO there's absolutely no need to refactor the orginal method. It's by far the most concise, readable version.

 

 

 

Michael Schnell replied on Wed, 2009/02/04 - 6:53am

I'm sorry but this is a good example of "overengineering".

The original code is short (only 20 Lines) easy to understand. Everyone knows what a "compareTo" method does and what the result values are.

The "refactored" code is large (48 lines) hard to understand and therefore not easy to maintain.

Don't follow blindly any "best practices" or "pattern"!

 

Ricky Clarkson replied on Wed, 2009/02/04 - 7:29am

public int compareTo(ExampleClass other) { return other.date.compareto(date); }

Any more is masturbatory.

Guillaume Jeudy replied on Wed, 2009/02/04 - 9:05am in response to: Ricky Clarkson

Right on, that works perfect and throws NPE if other is null which fulfills that part of the Comparable contract.

 Wondering why i didn't think about that earlier... You only need to keep track of 1, 0, -1 if you need to compare several fields.

Artur Biesiadowski replied on Thu, 2009/02/05 - 2:02am

I cannot see a point of having 'equals' call inside compareTo. If it makes a difference, you are asking for a trouble (it would mean two values which are 'equal', but comparison if allowed to run would make them non-equal).

By this 'refactoring' you have created a mess out of your class. Isn't it better to have static method somewhere which knows how to compare dates even if they are null and just write

 

  1. public int compareTo(final ExampleClass _other)throws ClassCastException, NullPointerException   
  2. {  
  3.    XYZUtil.compareDates(getCreatedDate(),_other.getCreatedDate();

This way you put reusable logic for dates in common place.

 

BTW, if you write comparators, you should write this.field.compareTo(other.field), not other way around as in your example. 

Kjetil Valstadsve replied on Thu, 2009/02/05 - 4:32am

First of all, this editor doesn't agree with me on what 'wysiwyg' means. Hope this formats readably.

The way I read the original method, you want to sort ExampleClasses (... please don't name your classes SomethingClass) so that equals() instances end up together, otherwise you sort them by their date. Finally, instances without dates should come first. Here's how I would do it:

public int compareTo(Object o) {
    return equals(o) ? 0
         : compare(this.date, ((ExampleClass)o).date);
}

private static int compare(Comparable one, Comparable two) {
    if (one == null && two == null) return 0;
    return one == null ? -1
        : two == null ? 1
            : one.compareTo(two)
}

The compare method needs to know that its arguments are comparable, but it doesn't need to know they're dates. Cue reuse.

ricky and gjeudy, about this one: (I corrected the order)

public int compareTo(ExampleClass other) {
    return date.compareto(other.date);
}

It does throw NPE if the other is null, but that also happens when you are a no-date instance. That is not part of the contract, is it - that's like saying this method fulfills the contract:

public int compareTo(ExampleClass other) {
    throw new NullPointerException();
}

Other than that, if that book has anything at all on class design, it probably has something on class invariants. In this case, assuming I havent' misunderstood something, I believe the whole case could be made simpler and more robust by enforcing a non-null invariant on ExampleClass by, say, defining 1970-01-01 as a Null Object:

private static final Date NO_DATE = new Date(0L);

private final Date date;

public ExampleClass(Date date) {
    this.date = date == null ? NO_DATE : date;
}

A static final field means you can check reliably for the no-date case using reference comparison. Knowing that date is never actually null reduces the compareTo to:

public int compareTo(Object o) {
    return equals(o) ? 0
        : this.date.compareTo(((ExampleClass)o).date);
}

This way, no-date ExampleClasses (I mean it, don't call your classes SomethingClass) naturally come first as well.

Bjørn Konestabo replied on Thu, 2009/02/05 - 5:42am

Seriously, Tomas! There is no polite way to say this, and I'll probaby get banned, but here goes:

Are you trying to discredit Robert Martins book, because this is one of the most misguided attempts at "refactoring" I've ever seen. Is this a parody article? Am I supposed to be laughing?

Instead I shudder to think that someone might be paying you to pervert code in this manner. I usually only see stuff like this on The Daily WTF.

I thought valstadsve's suggestion of using a Null Object was the best.

While you might find your refactored code more readable, there is certainly more of it to read, I find that chopping every expression or statement into a long winded method goes a long way of obfuscating the big picture. Can this possibly be a simple compareTo? Look at all this stuff. How can I be sure that all these methods do what the names imply? I'll have to read them all.

I'd prefer a clean one-liner to this convoluted mess any day of the week.

And one more thing: A code block does not an abstraction make.

Sheesh.

Tomas Malmsten replied on Thu, 2009/02/05 - 1:56pm

I am sure there is a polite way to say what you had to say Bjørn Konestabo, but you were not interested in finding one. But politeness aside I still feel that I need to respond.

I am not in any way trying to discredit Robert Martins work, and I do not think I do. By posting this article I have started a discussion about well written code. By starting the discussion I have received feedback on the structure of the code in the article. This is something that both I and other can learn from.

 

The initial code can't be very readable. If it was I assume that no-one would have missed it's intention and a few who responded did. This on the other hand also shows that the refactored code is to long winded to read since the same persons couldn't be bother to, so they still got it wrong. A null creation date is not allowed to throw an NPE.

valstadsve's solution is better then the initial method, beyond all doubt. However I have never agreed with the short hand if statements used in Java (the ? : statement). I find it very difficult to read and I know that I'm not alone. Also I find the short if(thisCreatedDateIsNull()) easier to read then the if(createdDate == null). So I think we will have to agree on disagreeing about that one.

I do like the null object. It makes the algorithm much easier to understand. I would move the creation of the null object to the getCreatedDate() method though.

I am sure that with careful though we would produce a much cleaner code then what is in the initial article. That s the whole point of the exercise. To start thinking about it and learning as we go along.

And yes, the name ExampleClass is stupid, especially for an article like this.

I hope the discussion will continue, and hopefully stay on a more polite level, as we go along. One of the most important things when critisising is to come with constructive suggestions on how to improve what is wrong in the previous material.

P.S valstadsve, the WYSIWYG editor is unfortunately not all to helpful. D.S

Kjetil Valstadsve replied on Thu, 2009/02/05 - 2:48pm in response to: Tomas Malmsten

There's not a lot to gain by moving the creation of the null object to the get method. It is a static final field, which must be initialized at class load time. Having a non-final static field, initialized lazily by an instance, would be quite  unconventional and would only confuse matters even further. Think about what static fields are, and the relationship between references and instances, and you will see my point.

As for the infamous :? construct, it is a matter of habit!

Bjørn Konestabo replied on Thu, 2009/02/05 - 4:09pm in response to: Tomas Malmsten

I assure you, there is no polite way, because only the foulest of language is strong enough to underline the level of clumsy ineptitude that you've demonstrated. I'm being as polite as I possibly can be.

I struggle to fathom why replacing well known java idioms with this homebaked folly can be seen as an improvement by anyone. Do you reside in a secluded enclave of likeminded who struggle equally to read the most trivial of codelines, or are everyone around you too polite to be truthful?

While we agree that the original method is less than ideal, an improvement would be to simplify it, not to frame every expression in a method describing the contents in plain english. You are writing comments in your method names, and comments of such a trivial nature that they are better left unwritten.

Being able to read code with ease is a skill. You should hone it rather than adding training wheels, especially wheels that are square. You are making things harder for everyone who do not struggle reading code, and anyone who is familiar with Java code.

I'm sorry you find me impolite, but it seems like the critique from other posters isn't sinking in, and I fear that readers might find your hamhandedness worthy of imitation. Something is terribly wrong with your perspective, and there is nothing I can say to correct it. It requires self reflection.

 

Fadzlan Bin Yahya replied on Tue, 2009/02/10 - 8:28pm

FYI, I read the book cover to cover.

I really hope you realise what you are doing here. I would argue that some things are better left untouched, no matter how much I agree on the book.

You see, best practices and patterns only make sense *when it makes sense*. There is no need to follow blindly. If the gain its not much, perhaps normal way is better and easier to understand. If you read the book, I think you realise as well, that even the authors breaks the rules when it seems to make sense doing so.

Bottom line is, design in software is always a question of compromise. And I really think what you have demonstrated here, is a bad compromise.

Snoobab Kicker replied on Thu, 2009/02/12 - 1:20pm

Unfortunately your refactorings have reduced the readability of this method. 

 

The best suggestions were the Null object pattern or helper method to handle the date comparisons, to hide the null dates etc. That way when reading the method you woud immediately see that an object is considered to be different in magnitude by virtue of being equal or the dates being different in magnitude.

 

public int compareTo(final Object _other) throws ClassCastException, NullPointerException {
final ExampleClass otherExampleClass = (ExampleClass) _other;
if (equals(otherExampleClass)) return 0;
return DateComparison.compare(this.getCreatedDate(), otherExampleClass.getCreatedDate());
}

 

Robert C. Martin replied on Wed, 2009/02/18 - 10:43am

I actuall prefer the following a bit more.  I think the OP might be a bit too much of a good thing.

  public int compareTo(final Object _other) throws Exception {
    ExampleClass otherExampleClass = (ExampleClass) _other;
    if (equals(otherExampleClass)) return 0;
    else return compareCreatedDates(getCreatedDate(), otherExampleClass.getCreatedDate());
  }

  private int compareCreatedDates(Date thisCreatedDate, Date otherCreatedDate) {
    if (thisCreatedDate == null && otherCreatedDate == null) return 0;
    if (thisCreatedDate == null && otherCreatedDate != null) return -1;
    if (thisCreatedDate != null && otherCreatedDate == null) return 1;
    return otherCreatedDate.compareTo(thisCreatedDate);
  }

I think this makes things pretty clear and untangles the if/else mess of the initial example, without proliferating the functions.  

Frankly, this example sits right at the limit.  If there were one more if statement in that second function, or if there were one more variable involved in the comparison, I would probably have gone with something closer to the OP solution.  But given that there are only two dates, and that the comparisons are very simple to decipher, I think it's better to limit the number of functions and keep the code a bit denser than the OP.

 

Llewellyn Falco replied on Wed, 2009/02/18 - 12:06pm

Personally i like this solution. I have removed the "effiency" check, and de-nulled the values.

public int compareTo(final Object that)throws ClassCastException, NullPointerException
{
Date thatDate = nullDateMeansBeginingOfTime(((ExampleClass) that).getCreatedDate());
Date thisDate = nullDateMeansBeginingOfTime(this.getCreatedDate());
return thisDate.compareTo(thatDate);
}

 

 I'm not completely sure that this exactly the same, having not seen the equals function, but unless it allows for 2 objects with different createdDates to be equal, it is. and the meaning is much cleared to me.

 

 

Bjørn Konestabo replied on Thu, 2009/02/19 - 7:05am in response to: Robert C. Martin

You've created a utility method that doesn't refer to any field and could be static.

Both the name of the method and input parameters are needlessly specific. Names should be chosen to reflect what a method does, not what you're going to use it for.

public int compareTo( Object other ) throws Exception {
ExampleClass otherExampleClass = (ExampleClass) other;
return nullsafeCompare( this.getCreatedDate(), other.createdDate() );
}

private static int nullsafeCompare( Comparable c1, Comparable c2 ) {
if ( c1 == c2 ) return 0;
if ( c1 == null ) return -1;
if ( c2 == null ) return 1;
return c1.compareTo( c2 );
}  

Kjetil Valstadsve replied on Thu, 2009/02/19 - 7:12am

Obviously, I roughly agree with Robert's proposal, having posted the same suggestion earlier in the thread! (I used the nefarious ?: construct though.) You should all consider Bjørn's points against using Date as a parameter type, and the name of the method. Is it really wise to introduce a connection between a field name and a convenience method? My refactoring tools wouldn't catch that.

I still think a class invariant (the date is final and never null) is the way to go for a class that is simpler and more robust for future maintenance.

Bjørn Konestabo replied on Tue, 2009/09/29 - 3:44am

I owe you an apology, Tomas. I implied you misrepresented Bob's book. Now I'm beginning to see that you represented it fairly.

Please do not take this as a condonement of this style of programming. 

Comment viewing options

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