IComparable implementation for a class representing a version

Question

I have an UpdateId class which represents some kind of version (like AssemblyVersion). UpdateId has three int values (listed in their compare priorities order): Year, ReleaseNumber and Revision.

How can I make my CompareTo method better? It looks clumpy to me.

public int CompareTo(object obj)
{
 if(obj == null) return 1;
 var updateId = obj as UpdateId;
 if (updateId != null)
 {
 int yearCompare = this.Year.CompareTo(updateId.Year);
 if (yearCompare == 0)
 {
 int releaseNumberCompare = this.ReleaseNumber.CompareTo(updateId.ReleaseNumber);
 if (releaseNumberCompare == 0)
 {
 return this.Revision.CompareTo(updateId.Revision);
 }
 return releaseNumberCompare;
 }
 return yearCompare;
 }
 else
 {
 throw new ArgumentException("Object is not an UpdateId");
 }
}

Answer 1

You could change it to reduce the "arrow style" like so

public int CompareTo(object obj)
{
 if(obj == null) { return 1; }
 var updateId = obj as UpdateId;
 if (updateId == null) 
 {
 throw new ArgumentException("Object is not an UpdateId");
 }
 int compareResult = this.Year.CompareTo(updateId.Year);
 if (compareResult != 0) { return compareResult; }
 compareResult = this.ReleaseNumber.CompareTo(updateId.ReleaseNumber);
 if (compareResult != 0) { return compareResult; }
 return this.Revision.CompareTo(updateId.Revision);
}

which will save you some horizontal spacing and some not needed variables, because they all meant the same so I changed them to compareResult.

Answer 2

As always, before you ask "how do I do this?", you should ask "has anybody already done this for me?"

public int CompareTo(object obj)
{
 if(obj == null) { return 1; }
 var other = obj as UpdateId;
 if(other == null) 
 { 
 throw new ArgumentException("Object is not an UpdateId", "obj"); 
 }
 return this.AsTuple().CompareTo(other.AsTuple());
}
private IComparable AsTuple()
{
 return Tuple.Create(Year, ReleaseNumber, Revision);
}

Note that this does have a potential performance overhead, so you should check anywhere that UpdateId is being used in a tight loop (which will generally be if it's being sorted). One potential performance improvement- if the Year, ReleaseNumber and Revision are immutable- would be to store the tuple in a private field so it does not need to be regenerated each time.

---

Note that as well as pushing the logic to an existing .NET class rather than trying to do it ourselves, we also:

• Invert the ArgumentException check. For some reason you do this for the null check but not the invalid argument type check or the ones that come later. As you can see from Heslacher's answer, this inversion makes the method much cleaner.
• We remove the redundant "this" keywords. Actually I did leave one in in the last line of CompareTo because I think it makes it read better. But usually, they just clutter things up if they're not needed.

---

Realistically, it's unlikely that you're ever going to write anything that consumes IComparable directly. Instead you're probably going to have .NET library methods do it for you. For this reason, consider using explicit interface implementation so that CompareTo doesn't clutter up your class's public interface (e.g. in IntelliSense) unnecessarily.

If you do want to be able to compare versions individually rather than as a sort operation (e.g. "Is the current version installed less than the latest version?"), you can implement the comparison operators as RobH suggests

• RobH: Similar to your answer recently where you used a Tuple to generate hash codes, I'm not sure if I love or hate this. IComparable comes up a lot in loops and all of the extra Tuples could make the GC work much harder than it has to... Which could not matter a jot in real world workloads...
• Ben Aaronson: @RobH In the other answer I mentioned explicitly that you should check for performance issues. Maybe I should mention it in this one too. You could always have the Tuple in a private field instead of regenerated each time, but it's the usual considerations about when to optimize performance
• RobH: Valid points. It's certainly a good trick for small classes.
• Ben Aaronson: @RobH Updated the answer to flag the performance aspect.

Answer 3

If you implement IComparable you probably want to also implement IComparable<T>, IEquatable<T>, ==, >, >=, < and <=. It's a bit of a pain but you'll find most things rely on each other:

public int CompareTo(object obj)
{
 if(obj == null) { return 1; }
 var updateId = obj as UpdateId;
 if (updateId == null)
 {
 throw new ArgumentException();
 }
 return CompareTo(updateId);
}
public int CompareTo(UpdateId updateId)
{
 // Shamelessly stolen from Heslacher
 int compareResult = this.Year.CompareTo(updateId.Year);
 if (compareResult != 0) { return compareResult; }
 compareResult = this.ReleaseNumber.CompareTo(updateId.ReleaseNumber);
 if (compareResult != 0) { return compareResult; }
 return this.Revision.CompareTo(updateId.Revision);
}