CODEWITHSUNDEEP
javacomparecompareto
user1016403
user1016403

Reputation: 12621

Best way to implement compare method of Comparator in java?

I have written a comparator which sorts by ascending order as below. which is working well.

Collections.sort(resultList,new Comparator<MyModelClass>() {
            @Override
            public int compare(MyModelClass o1, MyModelClass o2) {
                Integer id1= o1.getId();
                Integer id2= o2.getId();
                if(id1 == null && id2 == null) {
                    return 0;               
                }else if(id1 != null && id2 == null) {
                    return -1;
                } else if (id1 == null && id2 != null) {
                    return 1;
                } else {                
                    return id1.compareTo(id2);
                }
            }
        });

is it good to implement like this? Please help me?

Thanks!

Upvotes: 2

Views: 13921

Answers (5)

David Newcomb
David Newcomb

Reputation: 10943

No, it is not a good implementation.

The java.util.List specification says you can have nulls in a list and in some cases you can have multiple nulls. Your Comparator will fail with a NullPointerException as soon as it tries to do o?.getId() on a null element.

What I generally do is make my class implement java.lang.Comparable, then I can use a Map to sort the elements as I add them. One generally has to build the list so why not build a TreeMap instead?

If you are reusing your class and want to sort it in different ways you can create a TreeMap with a Comparator in the constructor so that there is no need to explicitly sort it.

Upvotes: 1

J&#246;rn Horstmann
J&#246;rn Horstmann

Reputation: 34014

If you need the null-safe comparison logic in several comparators then I would suggest using a static helper in a utility class like this:

public static int compare(Comparable c1, Comparable c2) {
    return c1 == null
               ? (c2 == null ? 0 : 1)
               : (c2 == null ? -1 : c1.compareTo(c2));
}

The Comparator could then be simplified to:

public int compare(MyModelClass o1, MyModelClass o2) {
    return CompareHelper.compare(o1.getId(), o2.getId());
}

Upvotes: 3

Per
Per

Reputation: 636

It looks good for readability, but a slightly more efficient way might be:

public int compare(MyModelClass o1, MyModelClass o2) {
    Integer id1= o1.getId();
    Integer id2= o2.getId();
    if (id1 == null) {
        return id2 == null ? 0 : 1;
    }
    if (id2 == null) {
        return -1;
    }
    return id1.compareTo(id2);
}

or even:

public int compare(MyModelClass o1, MyModelClass o2) {
    Integer id1= o1.getId();
    Integer id2= o2.getId();
    if (id1 == null) {
        return id2 == null ? 0 : 1;
    }

    return id2 == null ? -1 : id1.compareTo(id2);
}

Upvotes: 5

Shivan Dragon
Shivan Dragon

Reputation: 15219

Yes it is, I sort of do the same thing. One remark would be that you can use so nullsafe comparison tools such as Apache Commons Collections's NullComparator to ditch all those null checkings in your code:

http://commons.apache.org/collections/api-2.1.1/org/apache/commons/collections/comparators/NullComparator.html#compare(java.lang.Object, java.lang.Object)

Upvotes: 1

Egor
Egor

Reputation: 40203

If getId() returns an int, you can simply go with return id1.compareTo(id2), this will give you the right result. Hope this helps.

Upvotes: 1

Related Questions