CODEWITHSUNDEEP
javasortinglambdacomparatorcomparable
Arun Gowda
Arun Gowda

Reputation: 3530

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

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?

Upvotes: 9

Views: 12571

Answers (3)

Samuel Philipp
Samuel Philipp

Reputation: 11050

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)

Upvotes: 1

RedCam
RedCam

Reputation: 166

Put the call of reversed() inside thenComparing:

   Comparator.comparingInt(Amount::getLineNum)

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

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1503459

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.

Upvotes: 22

Related Questions