Lakatos Gyula Lakatos Gyula - 4 months ago 15
Java Question

Java error: Comparison method violates its general contract

I saw many questions about this, and tried to solve the problem, but after one hour of googling and a lots of trial & error, I still can't fix it. I hope some of you catch the problem.

This is what I get:

java.lang.IllegalArgumentException: Comparison method violates its general contract!
at java.util.ComparableTimSort.mergeHi(ComparableTimSort.java:835)
at java.util.ComparableTimSort.mergeAt(ComparableTimSort.java:453)
at java.util.ComparableTimSort.mergeForceCollapse(ComparableTimSort.java:392)
at java.util.ComparableTimSort.sort(ComparableTimSort.java:191)
at java.util.ComparableTimSort.sort(ComparableTimSort.java:146)
at java.util.Arrays.sort(Arrays.java:472)
at java.util.Collections.sort(Collections.java:155)
at magiccollector.gui.Main$1.actionPerformed(Main.java:85)
at org.pushingpixels.flamingo.api.common.AbstractCommandButton.fireActionPerformed(AbstractCommandButton.java:616)
at org.pushingpixels.flamingo.api.common.AbstractCommandButton$ActionHandler.actionPerformed(AbstractCommandButton.java:568)
at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:402)
at org.pushingpixels.flamingo.api.common.model.ActionRepeatableButtonModel.setPressed(ActionRepeatableButtonModel.java:123)
at org.pushingpixels.flamingo.internal.ui.common.BasicCommandButtonListener.mouseReleased(BasicCommandButtonListener.java:147)
at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:289)
at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:289)
at java.awt.Component.processMouseEvent(Component.java:6504)
at javax.swing.JComponent.processMouseEvent(JComponent.java:3321)
at java.awt.Component.processEvent(Component.java:6269)
at java.awt.Container.processEvent(Container.java:2229)
at java.awt.Component.dispatchEventImpl(Component.java:4860)
at java.awt.Container.dispatchEventImpl(Container.java:2287)
at java.awt.Component.dispatchEvent(Component.java:4686)
at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4832)
at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4492)
at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4422)
at java.awt.Container.dispatchEventImpl(Container.java:2273)
at java.awt.Window.dispatchEventImpl(Window.java:2713)
at java.awt.Component.dispatchEvent(Component.java:4686)
at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:707)
at java.awt.EventQueue.access$000(EventQueue.java:101)
at java.awt.EventQueue$3.run(EventQueue.java:666)
at java.awt.EventQueue$3.run(EventQueue.java:664)
at java.security.AccessController.doPrivileged(Native Method)
at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
at java.awt.EventQueue$4.run(EventQueue.java:680)
at java.awt.EventQueue$4.run(EventQueue.java:678)
at java.security.AccessController.doPrivileged(Native Method)
at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
at java.awt.EventQueue.dispatchEvent(EventQueue.java:677)
at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:211)
at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:128)
at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:117)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:113)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:105)
at java.awt.EventDispatchThread.run(EventDispatchThread.java:90)


And this is my comparator:

@Override
public int compareTo(Object o) {
if(this == o){
return 0;
}

CollectionItem item = (CollectionItem) o;

Card card1 = CardCache.getInstance().getCard(cardId);
Card card2 = CardCache.getInstance().getCard(item.getCardId());

if (card1.getSet() < card2.getSet()) {
return -1;
} else {
if (card1.getSet() == card2.getSet()) {
if (card1.getRarity() < card2.getRarity()) {
return 1;
} else {
if (card1.getId() == card2.getId()) {
if (cardType > item.getCardType()) {
return 1;
} else {
if (cardType == item.getCardType()) {
return 0;
}
return -1;
}
}
return -1;
}
}
return 1;
}
}


Any idea?

Answer

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!