Stream sorting issue

Question

I have a list of items (MyDetail object) that I want to sort with stream sorting methods. The object has 3 fields: field1, field2, field3. I want to sort by field3 first then field2 then field1, all with reversed order. So I wrote a method sortMyList.

I have a list of unsorted items unSortedDetails as the following: myDetail1: "20180201", false, false myDetail2: "20180101", false, false myDetail3: "20180101", false, true

after sortMyList(unSortedDetails), I expect my result would be myDetail3, myDetail1, myDetail2, but the actual result is myDetail1, myDetail3, myDetail2, Why?

so if I implement Comparable for MyDetail like the following, then it works as expected. this is so strange. I could not figure out why. thanks for any help!

public List<MyDetail> sortMyList(List<MyDetail> unSortedDetails){
    List<MyDetail> myDetails = unSortedDetails
                        .stream().sorted(Comparator.comparing(MyDetail::getField11).reversed()
                                .thenComparing(MyDetail::getField2).reversed()
                                .thenComparing(MyDetail::getField3).reversed())
                        .collect(Collectors.toList());
                        return myDetails;
                        }

                        @Setter
                        @Getter
                        public class MyDetail{
                            String field1;
                            Boolean field2; 
                            Boolean field3; 
                        }



                @Setter
                @Getter
                public class MyDetail implement Comparable<MyDetail>{
                    String field1;
                    Boolean field2; 
                    Boolean field3; 

                        @Override
                        public int compareTo(MyDetail o) {
                            if (this == o || this.equals(o)) return 0;
                            if (field3) return -1;
                            if (o.field3) return 1;
                            if (!field3 && !o.field3 && field2) return -1;
                            if(!field3 && !o.field3 &&!field2 && o.field2) return 1;
                            if(!field3 && !o.field3
                                    &&!field2 && !o.field2){
                                return o.field1.compareTo(field1);
                            }
                            return 0;
}
                }

Your field* fields are of the type String, but you're trying to treat them as boolean conditions in your if-statements. — Jacob G.
– Jacob G., Commented Feb 12, 2019 at 21:32
oh, Sorry, field3, field2 are boolean, field1 is string, updated — toosensitive
– toosensitive, Commented Feb 12, 2019 at 21:41

Pshemo · Accepted Answer · 2019-02-13 14:08:29Z

6

There are few problems with your Comparator. First is that each time you call reversed() you are reversing all previous settings of comparator.

So your comparator represents (see steps in comments, FieldX reduced to Fx)

Comparator.comparing(MyDetail::getField11)     //  F1 ASC
          .reversed()                          //~(F1 ASC) 
                                               //  F1 DESC 
          .thenComparing(MyDetail::getField2)  //  F1 DESC, F2 ASC  
          .reversed()                          //~(F1 DESC, F2 ASC) 
                                               //  F1 ASC,  F2 DESC
          .thenComparing(MyDetail::getField3)  //  F1 ASC,  F2 DESC, F3 ASC
          .reversed()                          //~(F1 ASC,  F2 DESC, F3 ASC)
                                               //  F1 DESC, F2 ASC,  F3 DESC

so you end up with order Field1 DESC, Field2 ASC, Field3 DESC.

To specify reversed order for each field do it by passing already reversed comparator of that field like .thenComparing(Comparator.comparing(YourClass::getField).reversed()).

Next issue is order of fields used by Comparator. In your question you said:

I want to sort by field3 first then field2 then field1

but your comparator first checks field1, then field2, then field3 (since in that order you added them via .thenComparing).

Your code should be more like

Comparator.comparing(MyDetail::getField13).reversed()
          .thenComparing(Comparator.comparing(MyDetail::getField2).reversed())
          .thenComparing(Comparator.comparing(MyDetail::getField1).reversed())

So you are creating ~(F3 ASC), ~(F2 ASC), ~(F1 ASC) which results in F3 DESC, F2 DESC, F1 DESC.

BTW as pointed by Holger you can achieve same effect by

Comparator.comparing(MyDetail::getField3)
          .thenComparing(MyDetail::getField2) 
          .thenComparing(MyDetail::getField1)
          .reversed()

Notice that Comparaor.comparing(FunctionToValue) and thenComparing(FunctionToValue) will create ASCending Comparators for selected Value.

So first 3 lines construct Comparator describing order F3 ASC, F2 ASC, F1 ASC. After reversing it
~(F3 ASC, F2 ASC, F1 ASC) also gives us desired F3 DESC, F2 DESC, F1 DESC.

edited Feb 13, 2019 at 14:08

answered Feb 12, 2019 at 21:45

Pshemo

125k26 gold badges194 silver badges280 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

toosensitive Over a year ago

thanks a lot! Your explanation is very clear. Love it!

toosensitive Over a year ago

One more question, so stream sort vs. comparable on class, which way is faster? I would vote the later but I tested and did not see much difference between the two.

Pshemo Over a year ago

I can't help you now but that sounds like an interesting question. Consider creating separate post for it and include your test. Anyway I probably wouldn't be bothered at first by performance since purpose of Comparable interface is to provide default order for elements, while purpose of Comparator is to let others use any custom made order. We usually don't need to think about performance of default order vs user-defined one, but like I said this may be interesting question for others to answer.

Holger Over a year ago

You’re overcomplicating things. To get a comparator using reverse order for all properties, just create a comparator for all properties and reverse the end result:

Comparator.comparing(MyDetail::getField3) .thenComparing(MyDetail::getField2) .thenComparing(MyDetail::getField1).reversed()

Pshemo Over a year ago

@Holger True, I focused mainly on explaining problems (misuse of reversed and wrong order of fields). But tbh while approach suggested by you is valid, IMO using thenComparing(reversedComparator) is easier to follow, so also less error-prone. Anyway thanks for pointing it out, will add your variant to the answer.

|

Collectives™ on Stack Overflow

Stream sorting issue

1 Answer 1

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related