13

I like the new static factory methods of Comparator, as they allow to implement Comparators in a very concise and less error-prone way.

But what is the recommended way to implement Comparable? Should we use Comparators inside the Comparable implementation?

public MyClass implements Comparable<MyClass>{
...
    public int compareTo(MyClass other){
        Comparator<MyClass> naturalOrderComparator = 
            Comparator.comparing(MyClass::getFoo)
                      .thenComparing(MyClass::getBar);
        return naturalOrderComparator.compare(this, other);
    } 
}

or even use a static comparator to reduce a lot of object creation when sorting huge collections:

public MyClass implements Comparable<MyClass>{
    private static final Comparator<MyClass> NATURAL_ORDER_COMPARATOR =     
        Comparator.comparing(MyClass::getFoo)
                  .thenComparing(MyClass::getBar);

    ...

    public int compareTo(MyClass other){
        return NATURAL_ORDER_COMPARATOR.compare(this, other);
    } 
}

Or is there another recommended way to implement Comparable with Java SE 8?

Stuart Marks
  • 127,867
  • 37
  • 205
  • 259
Puce
  • 37,247
  • 13
  • 80
  • 152
  • These are tiny convenience methods which you can easily implement yourself - calling `Comparator` statics is no shorter. For classes that need built-in compare methods, this is mostly pointless in terms of readability and brevity, not to mention performance. You can review the source source http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/Comparator.java#Comparator.comparingInt%28java.util.function.ToIntFunction%29 – pvg Sep 18 '17 at 19:05
  • @pvg what is the advantage of implementing utility methods myself over using the static methods of Comparator? Can you make a sample which is similar concise? – Puce Sep 18 '17 at 19:09
  • 4
    I would strongly recommend your second option. Writing these things yourself is likely to be more error prone. – Louis Wasserman Sep 18 '17 at 19:15
  • This is very much into 'primarily opinion based' territory but if you're implementing compareTo, you're also implementing equals and hashCode. Most classes don't need that, in the rare ones that do, you definitely want the clearest, most succinct and performant implementation. There is no benefit to using a convoluted `Comparator`-based implementation, those are great when you _don't_ need but just need a Comparator (as it says on the tin!) – pvg Sep 18 '17 at 19:16
  • @LouisWasserman: that's what I thought, too. But it somehow looks strange that with Java SE 8 the recommended way to implement Comparable is to use a Comparator. – Puce Sep 18 '17 at 19:17
  • @Puce why does it look strange? What's your concern, exactly? – dimo414 Sep 18 '17 at 19:18
  • @dimo414 it looks worse than 'strange'. If I bring up the `compareTo` method of some class, I want to know about the details of the ordering. Instead I'm greeted with N layers of abstraction and indirection at _precisely_ the places you don't want it. The method basically tells you 'what you want to find out is not here but in another castle'. I'd flag this type of implementation in a review. – pvg Sep 18 '17 at 19:24
  • @dimo414 Well, I never read somewhere something that this is the way to go, neither before nor after Java SE 8. – Puce Sep 18 '17 at 19:24
  • Hmm, I just double-checked the tutorial of Oracle. They seem to recommend a cascading call of the ternary operator: https://docs.oracle.com/javase/tutorial/collections/interfaces/order.html But then they're not using the static methods when explaining Comparator either. – Puce Sep 18 '17 at 19:26
  • I'm not sure treating the tutorial as a recommendation is the wisest course, _Effective Java_ doesn't cover Java 8, of course, although if you read the bit on `Comparable` you can make a reasonable guess as to what Joshua Bloch would suggest here. – pvg Sep 18 '17 at 19:32
  • 1
    @pvg, I've talked some with Josh Bloch, and I happen to have access to a copy of his current draft of the next version of Effective Java that discusses these issues. I would not assume the previous version's advice is still relevant in Java 8 =) – Louis Wasserman Sep 18 '17 at 19:34
  • @LouisWasserman you seem to be, though :) – pvg Sep 18 '17 at 19:35
  • 1
    @pvg "*I want to know about the details of the ordering.*" I'd contend that a manual implementation involving nested conditionals and custom arithmetic is much more difficult to parse than the utilities that abstract the details. To each their own, but I'd flag *your* code :) – dimo414 Sep 18 '17 at 19:38
  • 1
    I am. I can discuss the reasons for it, but they boil down to: I have seen dozens and dozens of ways to screw up handwriting `compareTo` methods, and very, very few ways to screw up the Java 8 comparator factories. They're simple, readable, and very difficult to get wrong. – Louis Wasserman Sep 18 '17 at 19:38
  • @LouisWasserman: could you formulate your recommendations as an answer? – Puce Sep 18 '17 at 19:40
  • @LouisWasserman I think the 80% answer is really 'you probably shouldn't be implementing `Comparable` anyway' so maybe we sort of agree. As to readable, I'm not seeing it. This `compareTo` straight up actually hides what is being compared. – pvg Sep 18 '17 at 19:41
  • 1
    @pvg: er.... foo and then bar? – steffen Sep 18 '17 at 20:07
  • @steffen neither of those are in the actual method. – pvg Sep 19 '17 at 00:36

2 Answers2

10

Your own Option 2 is almost certainly the best currently available way. It avoids allocation, it reads pretty well -- especially if you put the static constant next to the compareTo method.

The new Comparator.comparing factory methods in Java 8 are very easy to read, and even better, difficult to screw up -- there are many, many ways to write comparisons incorrectly by hand, more than I care to remember, and the factory methods are immune to most of them. Even though it's a little weird to use them to write a compareTo method instead of a Comparator object, it's still better than the alternatives.

Louis Wasserman
  • 191,574
  • 25
  • 345
  • 413
  • 3
    There are just two things to care about. 1.) the factory produced comparators don’t protect you from creating a logic that is inconsistent with equals, hence, you still have to care. 2.) you have to resist the temptation of creating null-safe comparators. While comparators may provide a formally correct logic for `null` values, comparables can not, as the contract requires symmetry, so `comparableInstance.compareTo(null)` must throw an exception, just like `(null).compareTo(comparableInstance)` would. – Holger Sep 19 '17 at 08:54
1

Pre-Java-8 the general best practice was to use Guava's ComparisonChain and Ordering utilities. They abstract away the cumbersome and easy-to-get-wrong details of properly implementing a .compareTo()/.compare() method, and allow you to compose a human-readable sequence of steps to define how objects should be compared. Ordering implements Comparator, but there'd be nothing wrong with defining an Ordering and invoking it in a Comparable's .compareTo() method.

Note that Ordering is described as obsolete thanks to the additions to the JDK in Java 8:

If you are using Java 8, this class is now obsolete.... Most of its functionality is now provided by Stream and by Comparator itself, and the rest can now be found as static methods in our new Comparators class.

dimo414
  • 47,227
  • 18
  • 148
  • 244