Java error: Comparison method violates its general contract
The exception message is actually pretty descriptive. The contract it mentions is transitivity: if A > B
and B > C
then for any A
, B
and C
: A > C
. I checked it with paper and pencil and your code seems to have few holes:
if (card1.getRarity() < card2.getRarity()) {
return 1;
you do not return -1
if card1.getRarity() > card2.getRarity()
.
if (card1.getId() == card2.getId()) {
//...
}
return -1;
You return -1
if ids aren't equal. You should return -1
or 1
depending on which id was bigger.
Take a look at this. Apart from being much more readable, I think it should actually work:
if (card1.getSet() > card2.getSet()) {
return 1;
}
if (card1.getSet() < card2.getSet()) {
return -1;
};
if (card1.getRarity() < card2.getRarity()) {
return 1;
}
if (card1.getRarity() > card2.getRarity()) {
return -1;
}
if (card1.getId() > card2.getId()) {
return 1;
}
if (card1.getId() < card2.getId()) {
return -1;
}
return cardType - item.getCardType(); //watch out for overflow!
Comparison method violates its general contract!
Your comparator is not transitive.
Let A
be the parent of B
, and B
be the parent of C
. Since A > B
and B > C
, then it must be the case that A > C
. However, if your comparator is invoked on A
and C
, it would return zero, meaning A == C
. This violates the contract and hence throws the exception.
It's rather nice of the library to detect this and let you know, rather than behave erratically.
One way to satisfy the transitivity requirement in compareParents()
is to traverse the getParent()
chain instead of only looking at the immediate ancestor.
Comparison method violates its general contract - how to avoid it
Your comparator doesn't deal with nulls and unparseable dates correctly. Consider the following case:
Suppose you have two non null dates d1 and d2 and a null d3.
Suppose d1 > d2.
You thus have
d1 > d2
d1 == d3
d2 == d3
So, if d1 and d2 are both equal to d3, they should also be equal to each other, but they're not.
Start by transforming all your strings to dates or null.
Then use a comparator which considers all null values as bigger (or lower) than all non null values. Comparator
has utility methods to transform a comparator of non-null objects into a comparator which deals with nulls by putting them all first or last.
Comparison method violates its general contract while comparing java.util.Date
The "contract" for comparison is that the comparison function has to define a total order. One of the requirements of a total order is "asymmetry", which basically means that if a < b, then b > a. In Java terms, this means that if compare(a,b)
(or a.compareTo(b)
) returns a result < 0, then compare(b,a)
(or b.compareTo(a)
) must return a result > 0. Your comparison function doesn't obey this rule; if x.getAttribute()
is non-null and y.getAttribute()
is null, then compare(x,y)
returns -1 and compare(y,x)
also returns -1.
TimSort sometimes notices this and throws an exception when it spots a comparison that doesn't return what it expects.
Another way to look at it: you have to decide beforehand what order you want things to be in if there are "special" values in the input (except that if you want objects to be compare as "equal", the order doesn't matter). Suppose your input contains objects whose getAttribute()
is null, and objects whose getAttribute()
is non-null. Where do you want the ones with the null attribute to appear in the output? How do you want them to be ordered? "I don't care" is not an option. Let's say you want all the null-attribute ones to come last, but you don't care about how the null-attribute objects are ordered. Then you need to write your comparison function so that
- an object with a non-null attribute < an object with a null attribute;
- an object with a null attribute > an object with a non-null attribute;
- two object with null attributes are treated as equal (comparison function returns 0).
If you want the null ones to appear first in the array, then the < and > in the first two points would be reversed. If you want two objects with null attributes to be ordered based on some other attribute, then write your comparison function so that it does that, but you'll still need to decide where the ones with the null attribute appear relative to the ones with the non-null attribute. Maybe it doesn't matter which one you choose. But you have to choose something, and you have to write your comparison function to return the result based on what you've chosen.
P.S.: There's no particular reason why your second code snippet with Employee
works and the first one doesn't. Your comparator in the second case is just as wrong as in the first one. However, TimSort doesn't look at every pair of elements to make sure the comparison meets the contract (that would make it an O(n2) algorithm). I'm not familiar with the details of TimSort, but I suspect that it only makes this check when it has a reason to compare two elements to see if the comparison is (perhaps) <0 or =0, and it "knows" that >0 should not be possible if the comparison function meets the contract. It's pretty cheap to check the result for >0 if it already has to make the comparison, but I doubt that TimSort calls comparators when it doesn't have to, since the execution time of a comparator is beyond its control. So the basic reason your second example works is "you got lucky".
“Comparison method violates its general contract!”
Both these cases are problematic.
if (lhs == null || rhs == null) return 0;
If you have [123, null, 234]
, then you compare 123
as equal to null
, null
as equal to 234
, and by transitivity you should get 123
equals 234
. But that is not what your comparator returns.
The solution here would be to either disallow null
or sort all nulls
to the bottom (or top), i.e. only return 0
is both are null
, otherwise return 1
or -1
(depending on the null
being left or right).
return (int) (lhs.mTaskInfo.time - rhs.mTaskInfo.time);
Consider comparing Integer.MAX_VALUE + 1
to 0
. The difference between the two is Integer.MAX_VALUE + 1
. Casting this to int
wraps to Integer.MIN_VALUE
. The opposite comparison should then give you - Integer.MIN_VALUE
, but that is Integer.MIN_VALUE
again due to overflow.
The solution here is to use Long.compare(a,b)
.
java.lang.IllegalArgumentException: Comparison method violates its general contract
Your compare()
method is not transitive. If A == B
and B == C
, then A
must be equal to C
.
Now consider this case:
For A
, B
, and C
, suppose the containsKey()
method return these results:
childMap.containsKey(A.getID())
returnstrue
childMap.containsKey(B.getID())
returnsfalse
childMap.containsKey(C.getID())
returnstrue
Also, consider orders for A.getId()
!= B.getId()
.
So,
A
andB
would return0
, as outerif
condition will befalse
=>A == B
B
andC
would return0
, as outerif
condition will befalse
=>B == C
But, A
and C
, could return -1
, or 1
, based on your test inside the if
block. So, A != C
. This violates the transitivity principle.
I think, you should add some condition inside your else
block, that performs check similar to how you do in if
block.
Exception 'Comparison Method Violates Its General Contract!' when comparing two date properties of an object
The only problem is how your handle null for primaryDate
.
Check the following example:
MyClass | primaryDate | secondaryDate |
---|---|---|
A | null | 1/1/2021 |
B | 1/1/2023 | 1/1/2022 |
C | 1/1/2024 | 1/1/2020 |
Collections.sort() Comparison method violates its general contract in Java
You should start by reading the JavaDoc of Comparator.compare() to understand what that "contract" is:
The implementor must ensure that sgn(compare(x, y)) == -sgn(compare(y,
x)) for all x and y.
In normal terms it says that if "x is greater than y, y must be smaller than x". Sounds obvious, but is not the case in your comparator:
- In your case you violate it when two Users have
checkMatchConditions
false
, in which casecompare(u1, u2)
andcompare(u2, u1)
both return 1. Hence, there are cases whereu1
is greater thanu2
, andu2
is greater thanu1
, which is a violation. - Similarely, if both Users have
checkMatchConditions
true
, and theirlastMatchDates
are bothnull
, they will also violate the contract. - In addition, because you manually try to compare the dates with
isBefore
, you also return -1 in both cases when two Users havecheckMatchConditions
true
and theirlastMatchDates
are both equal.
In order to fix this, you should first add a natural language description of how you want the Users to be ordered. Then you can work out the comparator logic.
The error has nothing to do with the Boolean.valueOf()
by the way.
Now that you explained how you want to order, have a look at this comparator:
public int compare(MiniUser u1, MiniUser u2)
{
// order by match
boolean u1Matches = checkMatchConditions(u1, user);
boolean u2Matches = checkMatchConditions(u2, user);
if (u1Matches != u2Matches)
{
// put matching ones first
return u1Matches ? -1 : 1;
}
else if (u1Matches)
{
// order by dates
boolean u1HasDate = u1.getLastMatchDate() != null;
boolean u2HasDate = u2.getLastMatchDate() != null;
if (u1HasDate != u2HasDate)
{
// put the ones without date first
return u1HasDate ? 1 : -1;
}
else if (u1HasDate)
{
// order chronologically
return u1.getLastMatchDate().compareTo(u2.getLastMatchDate());
}
else
{
// no dates, no order possible
return 0;
}
}
else
{
// both don't match, no order possible
return 0;
}
}
If I understood your requirements correctly, this should impose a consistent order to your elements. Note how I use Date
's compareTo
for the date ordering instead of doing it myself, and how I return 0
in case they are "equal" in regards to the order instead of "randomly" returning 1
or -1
.
Related Topics
How to Generate Exceptions from Repaintmanager
How to Save a String to a Text File Using Java
Get Type of a Generic Parameter in Java With Reflection
Get a List of Resources from Classpath Directory
How Are Ssl Certificate Server Names Resolved/How to Add Alternative Names Using Keytool
How to Change the Date Format in Java
Placing Component on Glass Pane
How to Send an Email by Java Application Using Gmail, Yahoo, or Hotmail
How to Split a String by Space
Does Gc Release Back Memory to Os
How to Concatenate Two Strings in Java
Convert Arraylist≪String≫ to String[] Array
MySQL Jdbc Driver 5.1.33 - Time Zone Issue
Method Has the Same Erasure as Another Method in Type