2

Some of our users are getting this exception while sorting a list. The code that throws it is 

Collections.sort(activeConverstions, new Comparator<Conversation>() {
        @Override
        public int compare(Conversation o1, Conversation o2) {
            return (int)(o2.getTime()- o1.getTime()); // descending order
        }
    });

while getTime() is of type "long"

Sayed Jalil Hassan
  • 2,535
  • 7
  • 30
  • 42
  • 1
    I may be wrong, but compare method should only return -1, 0 or 1 – DamianoPantani Aug 17 '16 at 12:16
  • possible duplicate: [Java error: Comparison method violates its general contract](http://stackoverflow.com/questions/11441666/java-error-comparison-method-violates-its-general-contract) – DimaSan Aug 17 '16 at 12:18

3 Answers3

6

The issue is probably casting a long to int, which may convert a large long number to a negative int.

For example, consider this snippet :

long first = Integer.MAX_VALUE + 1;
long second = 0;
System.out.println((int) (first - second));
System.out.println((int) (second - first));

Output :

-2147483648
-2147483648

If you would pass to your compare method two Conversation instances - let's call them x and y - whose getTime() is equal to first and second in the above snippet respectively, both compare(x,y) and compare(y,x) would return a negative value, which violates the contract of that method.

Try :

Collections.sort(activeConverstions, new Comparator<Conversation>() {
    @Override
    public int compare(Conversation o1, Conversation o2) {
        return o2.getTime() > o1.getTime() ? 1 :  o2.getTime() < o1.getTime() ? -1 : 0;
    }
});

or, as assylias suggested :

Collections.sort(activeConverstions, new Comparator<Conversation>() {
    @Override
    public int compare(Conversation o1, Conversation o2) {
        return Long.compare(o2,getT‌​ime(), o1.getTime());
    }
});
Eran
  • 387,369
  • 54
  • 702
  • 768
2

A best practice of implementing compare method is to only return -1, 0 or 1.

    Collections.sort(activeConverstions, new Comparator<Conversation>() {
        @Override
        public int compare(Conversation o1, Conversation o2) {
            long result = o2.getTime()- o1.getTime();
            return result < 0 ? -1 : result > 0 ? 1 : 0;
        }
    });

EDIT:

As @assylias mentioned it's even better to use already implemented compare methods:

    Collections.sort(activeConverstions, new Comparator<Conversation>() {
        @Override
        public int compare(Conversation o1, Conversation o2) {
            return Long.compare(o2,getT‌​‌​ime(), o1.getTime());
        }
    });
DamianoPantani
  • 1,168
  • 2
  • 13
  • 23
  • 2
    Thanks. the official docs says it returns "a negative integer, zero, or a positive integer as the first argument is less than, equal to, or greater than the second" but i think casting long to int might be a problem. will give it a try. Thanks – Sayed Jalil Hassan Aug 17 '16 at 12:21
  • 1
    By 'should' I meant a best practice. A little misunderstanding. Edited. – DamianoPantani Aug 17 '16 at 12:23
  • 1
    It is not good practice to subtract numbers in a comparator because the subtraction may overflow (even if it's unlikely in this case). You should use `return Long.compare(o2,getT‌​ime(), o1.getTime());` instead or compare the times directly instead of calculating the difference. – assylias Aug 17 '16 at 12:31
  • @assylias Yup, +1, just editing and giving you credits – DamianoPantani Aug 17 '16 at 12:35
2

You can try this 

 Collections.sort(activeConverstions, 
    new Comparator<Conversation>()            {
    @Override
    public int compare(Conversation o1, Conversation o2) {
    if ((o2.getTime() > o1.getTime())
      return 1;
    else  if ((o2.getTime() < o1.getTime())
     return -1;
    else return 0;
    }
 });
Don
  • 227
  • 1
  • 7