Comparator .comparing().reversed() strange behaviour / not working as expected

Question

As per my knowledge, Comparator.comparingInt() should sort in ascending order and Comparator.comparingInt().reversed should sort in descending order. But I found a scenario where this is inverse.

This is better explained with an example. Following is my code.

Amount class:

class Amount
{
    int lineNum;
    int startIndex;
    Double value;
//Getters , setters and toString.
}

Main method:

public static void main( String[] args )
{
    List<Amount> amounts = new ArrayList<>();
    amounts.add( new Amount( 1.0, 5, 10 ) ); //LINE_NUM 5
    amounts.add( new Amount( 3.0, 9, 30 ) );
    amounts.add( new Amount( 2.0, 3, 40 ) );
    amounts.add( new Amount( 9.0, 5, 20 ) ); //LINE_NUM 5
    amounts.add( new Amount( 6.0, 1, 50 ) );
    amounts.add( new Amount( 4.0, 5, 20 ) ); //LINE_NUM 5
    System.out.println( ".............BEFORE SORTING.........." );
    amounts.forEach( System.out::println );


    amounts.sort( 
                 Comparator.comparingInt( Amount::getLineNum )   //NOTE THIS
        .           .thenComparingInt( Amount::getStartIndex ).reversed()
                      .thenComparingDouble( Amount::getValue ) );

    System.out.println( "\n\n.............AFTER SORTING.........." );

    amounts.forEach( System.out::println );
}

I wanted amount list sorted by lineNum ascending, by startIndex descending and value ascending.

So my expectation was this.

.............AFTER SORTING..........(EXPECTATION)

Amount [lineNum=1, startIndex=50, value=6.0]

Amount [lineNum=3, startIndex=40, value=2.0]

Amount [lineNum=5, startIndex=20, value=4.0]

Amount [lineNum=5, startIndex=20, value=9.0]

Amount [lineNum=5, startIndex=10, value=1.0]

Amount [lineNum=9, startIndex=30, value=3.0]

.............AFTER SORTING..........(ACTUAL)

Amount [lineNum=9, startIndex=30, value=3.0]

Amount [lineNum=5, startIndex=20, value=4.0]

Amount [lineNum=5, startIndex=20, value=9.0]

Amount [lineNum=5, startIndex=10, value=1.0]

Amount [lineNum=3, startIndex=40, value=2.0]

Amount [lineNum=1, startIndex=50, value=6.0]

Everything was right except for lineNum order. Amounts were sorted by lineNumber descending while I was expecting it to be in ascending order.

The results were as expected when I changed the Comparator to following

amounts.sort(
    Comparator.
    comparingInt( Amount::getLineNum ).reversed()
    .thenComparingInt( Amount::getStartIndex ).reversed()
    .thenComparingDouble( Amount::getValue ) );

Which is strange because, comparingInt( Amount::getLineNum ).reversed() was supposed to sort amounts by line number descending .

One more thing I noticed is, Comparing by StartIndex works as expected. But Comparing by lineNumber part is not.

Can somebody explain this?

score 21 · Accepted Answer · answered Apr 03 '19 at 17:00

21

It's easier to understand what's going on if you put each call on a line:

Comparator.comparingInt(Amount::getLineNum)
    .thenComparingInt(Amount::getStartIndex)
    .reversed()
    .thenComparingDouble(Amount::getValue)

That reversed() returns a comparator which reverses the results of the comparator it's called on... which is "the comparator which first compares the line number, then the start index." It's not like it's "bracketed" to just the scope of the previous thenComparingInt() call, which is how your previous formatting made it look like.

You could do it as:

Comparator.comparingInt(Amount::getLineNum)
    .thenComparing(Comparator.comparingInt(Amount::getStartIndex).reversed())
    .thenComparingDouble(Amount::getValue)

At that point it's only the start index comparison that's reversed.

answered Apr 03 '19 at 17:00

Jon Skeet

1,421,763
867
9,128
9,194

Do you mean, the reversed() on any comparator applies to all Previous comparators as well? – Arun Gowda Apr 03 '19 at 17:03
@ArunGowda it gets applied to the result of the `thenComparingInt` method, that is a `Comparator` *incorporating* the previous ones – user85421 Apr 03 '19 at 17:26
1

@ArunGowda: You call it on *a comparator*. It reverses the result of that comparator. The comparator produced by `Comparator.comparingInt(Amount::getLineNum).thenComparingInt(Amount::getStartIndex)` is "a comparator that orders by line number then start index". So when you call `reverse` on that comparator, it returns "(a comparator that orders by line number then start index) - but in a reverse order" – Jon Skeet Apr 03 '19 at 17:26
maybe easier to understand: `comp1 = Comparator.comparingInt(Amount::getLineNum);` `comp2 = comp1.thenComparingInt(Amount::getStartIndex);` `comp3 = comp2.reversed();` – user85421 Apr 03 '19 at 17:30
I'm trying to use this, but I'm in the case where the first and second comparators are reversed, only the third one is not. In this case, it seems `Comparator.comparing(Comparator.comparingInt(Amount::getLineNum).reversed()).thenComparing...` does not work anymore. I have to use `reversed`outside the parenthesis: `Comparator.comparingInt(Amount::getLineNum).reversed().thenComparing...`. Why is it possible for `thenComparing` but not for `comparing`? I understand it's different method signatures, I'm just wondering if I do it the right way. – fidekild Apr 12 '21 at 16:31
@fidekild: I think this would be best demonstrated in a new question with a [mcve]. – Jon Skeet Apr 12 '21 at 17:18

score 1 · Answer 2 · edited Jun 20 '20 at 09:12

From The docs:

reversed(): Returns a comparator that imposes the reverse ordering of this comparator.

thenComparing(): Returns a lexicographic-order comparator with another comparator. If this Comparator considers two elements equal, i.e. compare(a, b) == 0, other is used to determine the order.

Each step creates a new comparator based on the previous one. So the reversed() method creates a reversed comparator of

Comparator.comparingInt(Amount::getLineNum).thenComparingInt(Amount::getStartIndex)

To get only the second one reversed you should wrap it in an own comparator:

.thenComparing(Comparator.comparingInt(Amount::getStartIndex).reversed())

In your second solution the result is correct, because you are actually reversing the first condition twice:

Comparator.comparingInt(Amount::getLineNum).reversed() // reverses one time
    .thenComparingInt(Amount::getStartIndex).reversed() // reverses all before (also the first one)

So the complete solution would look like this:

Comparator.comparingInt(Amount::getLineNum)
    .thenComparing(Comparator.comparingInt(Amount::getStartIndex).reversed())
    .thenComparingDouble(Amount::getValue)

score 1 · Answer 3 · answered Apr 03 '19 at 17:14

1

Put the call of reversed() inside thenComparing:

   Comparator.comparingInt(Amount::getLineNum)

   .thenComparing(Comparator.comparingInt(Amount::getStartIndex).reversed())
   .thenComparingDouble( Amount::getValue );

answered Apr 03 '19 at 17:14

RedCam

166
2

Comparator .comparing().reversed() strange behaviour / not working as expected

3 Answers3

Linked