Java: IllegalArgumentException

Question

I am looking at some legacy code and I found a section that is causing me to get a “Comparison method violates its general contract!” error. I understand that this error is the result of the code not being transitive, but I don't fully understand how to fix it correctly.

Here is the code responsible for the error.

private void sortHistories(List<History> histories) {
        Collections.sort(histories, new Comparator<History>() {

            @Override
            public int compare(History o1, History o2) {

                return o1 == o2 ? 0
                        : o1 == null ? -1
                        : o2 == null ? 1
                        : o1.getFamilyMembers().equals(o2.getFamilyMembers()) ? 0 //getFamilyMembers() returns a string
                        : o1.getFamilyMembers() == null ? -1
                        : o2.getFamilyMembers() == null ? 1
                        : o2.getFamilyMembers().compareTo(o2.getFamilyMembers()) != 0 ?
                                o2.getFamilyMembers().compareTo(o2.getFamilyMembers())
                        : o1.getDisease().equals(o2.getDisease()) ? 0 //getDisease() also returns a string
                        : o1.getDisease() == null ? -1
                        : o2.getDisease() == null ? 1
                        : o1.getDisease().compareTo(o2.getDisease());
            }
        });
    }

Originally, the code was using == rather than equals() when comparing the strings getDisease() and getFamilyMembers(). I thought making the change from == to equals() would fix the problem, but that is not the case.

you have to check o1.getDisease() equals to null or not before comparing os.getDisease() whether equals to o2.getDisease() — Haifeng Zhang, Dec 26 '16 at 05:04
The `==` was an optimization. Two lines below, `o2.getFamilyMembers().compareTo(o2.getFamilyMembers()) != 0` does the real work. So `==1 *was* correct (and redundant). — David Ehrmann, Dec 26 '16 at 05:27
@HaifengZhang I have tried that, in addition to moving another null check, but I am still getting the error. I have posted my updated code above. — bob dylan, Dec 26 '16 at 05:36
@DavidEhrmann So I should get ride of `o1.getFamilyMembers().equals(o2.getFamilyMembers()) ? 0` and everything else will be fine? — bob dylan, Dec 26 '16 at 05:41
I do not see anything like `o1.getFamilyMembers().equals(o2.getFamilyMembers())` but I do see `o2.getFamilyMembers().compareTo(o2.getFamilyMembers())` is this correct? Because this will always return `0`. — YoungHobbit, Dec 26 '16 at 05:43
@HaifengZhang It seems to be working now with both your suggestion and the other persons! Thank you! — bob dylan, Dec 26 '16 at 06:04
Please do add your solution as answer for the completeness of the post. :) — YoungHobbit, Dec 26 '16 at 06:09
Adding the solution into Question is not a right thing, you should post it as Answer. Thanks.:) — YoungHobbit, Dec 26 '16 at 08:08
@YoungHobbit Ok. I just didn't want it to seem that I was stealing your guys' credit. I have posted it as an answer now. — bob dylan, Dec 26 '16 at 22:09

score 2 · Answer 1 · answered Dec 26 '16 at 22:08

The solution, thanks to HaifengZhang and YoungHobbit, is:

public int compare(History o1, History o2) {
                return o1 == o2 ? 0
                        : o1 == null ? -1
                        : o2 == null ? 1
                        : o1.getFamilyMembers() == null ? -1
                        : o2.getFamilyMembers() == null ? 1
                        : o1.getFamilyMembers() == o2.getFamilyMembers() ? 0
                        : o2.getFamilyMembers().compareTo(o1.getFamilyMembers()) != 0 ?
                                o2.getFamilyMembers().compareTo(o1.getFamilyMembers())
                        : o1.getDisease() == null ? -1
                        : o2.getDisease() == null ? 1
                        : o1.getDisease() == o2.getDisease() ? 0
                        : o1.getDisease().compareTo(o2.getDisease());
            }

score 0 · Answer 2 · edited May 23 '17 at 12:33

0

Read the documentation carefully.Throwing this exception is a new Java7 feature.

More details here :: https://stackoverflow.com/a/8327575/3080158

The old behavior can be configured with a new system property: java.util.Arrays.useLegacyMergeSort.

http://www.oracle.com/technetwork/java/javase/compatibility-417013.html#source

http://docs.oracle.com/javase/7/docs/api/java/util/Comparator.html

edited May 23 '17 at 12:33

Community

1
1

answered Dec 26 '16 at 05:26

RBanerjee

957
1
9
18

Agree that this may get rid of the exception, but it won't fix the sort order, will it? – Erwin Bolwidt Dec 26 '16 at 05:39
Yes, to fix the sort order compactor should follow the contract,i.e for lt -1 , gt 1, and equals 0 , with transitivity. – RBanerjee Dec 26 '16 at 05:43
That's clear, so, how does the OP do that given the code that was posted? – Erwin Bolwidt Dec 26 '16 at 05:53
I didn't tried to solve the problem particularly, but if you go through the threads and docs ,the rule of thumb will be `check equality (a==b ? 0), then null(a or b is null -1,1),then compare or repeat for child` . If you check null first for example a==null? -1 then it might happen that b is also null , for which you should actually return 0. – RBanerjee Dec 27 '16 at 04:54

Java: IllegalArgumentException

2 Answers2