CODEWITHSUNDEEP
javasortingdictionary
nourani
nourani

Reputation: 69

Custom Comparator for ConcurrentSkipListMap

I need to define a customized comparator to ConcurrentSkipListMap, I use this code to sort based on 'LogicalClock' but the result is not as I expected. I create the key like this: "ClientId"+":"+"LogicalClock"

class Qentry{
    int  AckCount;
    int ClientID; 
    long LogicalClock;
}

Comparator<String> LogicalClockComparator = new Comparator<String>() {
        @Override public int compare(String k1, String k2) {
            if  (k1.compareTo(k2)==0)
                    return 0;
            return   (int)( Long.valueOf(k1.substring(k1.indexOf(":")+1)) -Long.valueOf(k2.substring(k1.indexOf(":")+1) ));
        }
ConcurrentSkipListMap<String,Qentry> q;
q =new ConcurrentSkipListMap<String,Qentry>(LogicalClockComparator);

Upvotes: 0

Views: 1003

Answers (3)

assylias
assylias

Reputation: 328608

Apart from the typo, note that you have 2 potential sources of overflow: when subtracting the 2 longs and when casting to int. It would probably be better to use:

Long value1 = Long.valueOf(k1.substring(k1.indexOf(":")+1));
Long value2 = Long.valueOf(k2.substring(k2.indexOf(":")+1));
return value1.compareTo(value2);

Upvotes: 1

Peter Lawrey
Peter Lawrey

Reputation: 533510

This would have surprising results for large time differences. compare should only return -1, 0, or +1 but you can get away with larger ranges. For long you are like to get an overflow casting to an int which would have unexpected behaviour.

I suggest you use Long.compare() if its available and Double.compare if it is not.

BTW, as Maps don't allow duplicate keys, when you return 0, it treats it a as a duplicate so if you have a:1, b:1 and c:1 they are all duplicates. The way around this is to compare the whole String if the comparison is equal.

BTW2 While this is pretty inefficient code you could use parseLong instead of valueOf to improve it slightly.

Upvotes: 1

jlordo
jlordo

Reputation: 37813

Looks like a typo (or copy/paste error) to me, maybe you want to use (look at the end of the line)

return   (int)( Long.valueOf(k1.substring(k1.indexOf(":")+1)) -Long.valueOf(k2.substring(k2.indexOf(":")+1) ));

instead of what you had:

return   (int)( Long.valueOf(k1.substring(k1.indexOf(":")+1)) -Long.valueOf(k2.substring(k1.indexOf(":")+1) ));

Upvotes: 2

Related Questions