Replace CompareToBuilder with Java 8's Comparator.comparing(...).thenComparing(...)

Question

Before Java 8, we implemented Comparable.compareTo(...) like this:

public int compare(Person a, Person b) {
    return new CompareToBuilder()
            .append(a.getLastName(), b.getLastName())
            .append(a.getFirstName(), b.getFirstName())
            .toComparison();
}

As of Java 8, we can do it like this:

public int compare(Person a, Person b) {
    return Comparator
            .comparing(Person::getLastName)
            .thenComparing(Person::getFirstName)
            .compare(a, b);
}

The new Java 8 way might allow us to drop the commons-lang3 dependency. Is that new Java 8 way faster? Is there a way to automatically migrate? I didn't find an IntelliJ intention for it.

Notice that it becomes a bit more complex when there are reverse orders and non natural comparison is involved:

public int compare(SingleBenchmarkResult a, SingleBenchmarkResult b) {
    return new CompareToBuilder()
            .append(b.hasAnyFailure(), a.hasAnyFailure()) // Reverse
            .append(a.getAverageScore(), b.getAverageScore(), resilientScoreComparator)
            .toComparison();
}

becomes

public int compare(SingleBenchmarkResult a, SingleBenchmarkResult b) {
    return Comparator
            .comparing(SingleBenchmarkResult::hasAnyFailure, Comparator.reverseOrder()) // Reverse
            .thenComparing(SingleBenchmarkResult::getAverageScore, resilientScoreComparator)
            .compare(a, b);
}

score 8 · Answer 1 · answered Jul 04 '16 at 12:36

If you write it this way

public int compare(Person a, Person b) {
    return Comparator
            .comparing(Person::getLastName)
            .thenComparing(Person::getFirstName)
            .compare(a, b);
}

you are wasting performance by constructing a new Comparator for each comparison. And it should be obviously nonsensical when looking at the surrounding code. The compare(Person a, Person b) method surely is part of a class implementing Comparator<Person>, which you instantiate at some place to get the desired comparator. You should replace that instance by a sole Comparator.comparing(Person::getLastName).thenComparing(Person::getFirstName) instance instead, used throughout the entire operation.

E.g.

// reusable
static final Comparator<Person> By_NAME = Comparator
             .comparing(Person::getLastName).thenComparing(Person::getFirstName);

or ad hoc

listOfPersons.sort(Comparator.comparing(Person::getLastName)
                             .thenComparing(Person::getFirstName));

If you use it that way, it’s very likely to be faster. However, you should see, that there is no simple pattern-based replacement possible. You have to replace the use sites of the class with that simple declarative construct and make a decision whether to use a shared comparator instance for multiple use sites or create it ad-hoc. Then, you can remove the entire old implementation class or at least, remove the comparator functionality from it if it still serves other purposes.

I need to implement the natural comparison of Person, so I can't apply these suggested changes. Do you believe Comparator.comparing(...) is also slower than CompareToBuilder? — Geoffrey De Smet, Jul 04 '16 at 16:16
If you implement the natural order, the method should be `compareTo(Person)` rather than `compare(Person,Person)`, so the question was misleading. I don’t think that using `Comparator.comparing` will be slower than `CompareToBuilder`, but still, you can improve it by declaring the `static final` field as shown in my answer and implement `compareTo(Person)` as `return BY_NAME.compare(this,other);`. — Holger, Jul 04 '16 at 16:20
Good point. Actually I have use cases where it's natural order and I have use cases where it's a reusable Comperator (like above). I see why it's no longer needed to make that specialized Comperator a separate class. — Geoffrey De Smet, Jul 04 '16 at 16:23

score 3 · Answer 2 · edited May 23 '17 at 12:22

I don't think there is any pre-defined inspection for that. You might try to use IntelliJ's structural-search, although I think it might by quite tricky to do that for every possible case. One possibility for a simple case with two comparisons might be the following:

search template (occurence count of $TYPE$ and $z$ is 2):

$ReturnType$ $MethodName$($TYPE$ $z$) {
        return new CompareToBuilder()
                .append($A$.$m$(), $B$.$m$())
                .append($A$.$m1$(), $B$.$m1$())
                .toComparison();
    }

replacement template:

$ReturnType$ $MethodName$($TYPE$ $z$) {
    return java.util.Comparator
            .comparing($TYPE$::$m$)
            .thenComparing($TYPE$::$m1$)
            .compare($A$, $B$);
}

I am not an expert on structural search, but I guess you would have to make another pattern for calls with more or less comparisons.

Replace CompareToBuilder with Java 8's Comparator.comparing(...).thenComparing(...)

2 Answers2