Version comparison function

Question

I have this version comparison function in java.

Any cleaner way to write this without all the nested 'if' statements?

Note that the suggestions must be in Java code only.

@Override
public int compareTo(Version o)
{
 int result = this.major.compareTo(o.major);
 if (result == 0)
 {
 result = this.minor.compareTo(o.minor);
 if (result == 0)
 {
 result = this.minor2.compareTo(o.minor2);
 if (result == 0)
 {
 result = this.suffix.compareTo(o.suffix);
 }
 }
 }
 return result;
}

Edit:

Just to be clear on the 'type' of the fields of the Version class:

Integer major, minor, minor2;
String suffix;

• user unknown: The nesting pretty much reflects the cascading structure of the data - why do you want to get rid of it? PHP-Coder shows an legitimate solution but I wouldn't necessarily prefer it.
• Inanc Gumus: If your only concern is readability you may convert the version to a string and then just compare.
• pdeva: with a pure string comparison, 10.0.0 will be less than 9.0.0.
• sunil: I just found a Java Library (Java SemVer) that supports the Semantic Versioning Specification. Download the library/source and use it. The usage is pretty simple. Version v1 = Version.valueOf("1.2.3"); Version v2 = Version.valueOf("1.2.0"); System.out.println(v1.compareTo(v2));

Answer 1

If you expect often, that two versions will be the same reference, you could start your method with a check for that. And you may strip the noisy 'this'. However, the cascading nature of the check is pretty fine expressed in the nesting - I would keep it.

@Override
public int compareTo (Version o)
{
 if (this == o)
 return 0;
 int result = major.compareTo (o.major);
 if (result == 0)
 {
 result = minor.compareTo (o.minor);
 if (result == 0)
 {
 result = minor2.compareTo (o.minor2);
 if (result == 0)
 {
 result = suffix.compareTo (o.suffix);
 }
 }
 }
 return result;
}

Answer 2

One point which I may suggest is reorder if statements to reduce indentation:

@Override
public int compareTo(Version o)
{
 int result = this.major.compareTo(o.major);
 if (result != 0)
 {
 return result;
 }
 result = this.minor.compareTo(o.minor);
 if (result != 0)
 {
 return result;
 }
 result = this.minor2.compareTo(o.minor2);
 if (result != 0)
 {
 return result;
 }
 return this.suffix.compareTo(o.suffix);
}

Also, I think you can omit this keyword.

• user unknown: I agree about the suffix, and because of the premature return, no else is needed. But the cascading nature of the problem is imho clearer to see from nested structure.

Answer 3

In the apache.common package (very usefull by the way)

There is a CompareToBuilder. Works like a charm (like the ToStringBuilder)

http://commons.apache.org/lang/api-2.6/org/apache/commons/lang/builder/CompareToBuilder.html

• pdeva: cant add dependency to a 3rd party library for a small function. Any improvement will have to be a refactoring of the code.
• Drahakar: Trust me, including this package will help your code on many different level, take a look at all the functions it offer. If you really can't include it in your code, then it is a good proof of concept that it can be done with reflexion. Answer to the question: So you could avoid all those if with reflexion.
• Drahakar: The down side is that reflexion is quite slow, compared with "normal" code.
• Winston Ewert: @OJ, bloat is all the code out there implementing this exact pattern without using a common code to do so.
• OJ.: @Winston: Sorry, but I think that's utter rubbish. I don't need to construct a class, fill it with stuff and call a method just to compare a few elements together. Bloat is not "lack of code reuse", bloat is writing wrappers for extremely small things like this. Seriously, comparing internal values does not require "infrastructure" or "builder" classes like this. If you feel compelled to use them, fine, so long as I don't have to maintain it. I don't agree with rubbish like this being taught to others though.

Answer 4

I like @php-coder's approach best, but I'd get rid of the unnecessary braces for even greater readability:

@Override public int compareTo(Version o) {
 if (this == o) return 0;
 int result = major.compareTo(o.major);
 if (result != 0) return result;
 result = minor.compareTo(o.minor);
 if (result != 0) return result;
 result = minor2.compareTo(o.minor2);
 if (result != 0) return result;
 return suffix.compareTo(o.suffix); 
}

Your compareTo method assumes that none of the fields can be null; watch out for that if you don't know for a fact it's always true.

• Stephen C: IMO it is a correct for the code to assume that o is non-null. It is best practice to assume that it is a programming error if o or a component field is null ... unless the javadocs clearly say otherwise.

Answer 5

This might be a little bit over-the-top, but how about a more object oriented approach? Assume this little helper enum:

public enum Compare {
 LT, EQ, GT;
 public int toInt() {
 switch(this) {
 case LT: return -1;
 case EQ: return 0;
 case GT: return 1;
 default: throw new AssertionError(); 
 } 
 }
 public Compare cp(int v1, int v2) {
 if(this == EQ) return v1 < v2 ? LT : v1 > v2 ? GT : EQ;
 else return this;
 }
 //... here the cp methods for other primitives 
 public <T extends Comparable<T>> Compare cp(T v1, T v2) {
 if(this == EQ) {
 int result = v1.compareTo(v2);
 return result < 0 ? LT : result > 0 ? GT : EQ;
 } else {
 return this;
 } 
 }
}

Then we can write:

import static somepackage.Compare.*;
@Override
public int compareTo(Version that) {
 return EQ.cp(this.major, that.major)
 .cp(this.minor, that.minor)
 .cp(this.minor2, that.minor2)
 .toInt();
}

Note that unnecessary comparisions will be skipped in the cp methods.

[Update]

Apache Commons does something similar: http://commons.apache.org/lang/api-2.5/org/apache/commons/lang/builder/CompareToBuilder.html

• pdeva: you are right. it is a bit over the top ;)
• Landei: I think it depends on the context. I wouldn't use this in performance critical code. However if I had a lot of big business objects, it could be more important to get comparision right than fast, and then this approach could be helpful.