5
\$\begingroup\$

I am trying to create a class which implements the Comparator class.

The thing is that I have a list with some objects. This object is composed of 3 fields (Strings). My purpose is to sort the list based first on the first field: if the first field is the same on the two objects that I am comparing at the moment, then compare the second item of the 2 objects and finally if both fields are the same, compare the third field of the objects.

This is what I have done but I do not know if this is the correct answer:

public class MyComparator implements Comparator<Object>{

    @Override
    public int compare(final Dto address1, final Dto address2) {
        int result = 0;

        if (dto instanceof Dto) {
            final Dto dto = (Dto) address;
            final Dto dto2 = (Dto) address2;
            if(dto.getName() < dto2.getName()) {
                 result = -1;
            } else if(dto.getName() > dto2.getName()) {
                result = 1;
            } else {
                result = 0;
            }
        } 
//if the Names are the same then compare the Number
            if (result == 0) {
                if(dto.Number() < dto2.Number()) {
                    result = -1;
                } else if(dto.Number() > dto2.Number()) {
                    result = 1;
                } else {
                    result = 0;
                }
            } 
        //if the Numbers are the same then compare the Other Field

        if (result == 0) {
            if(dto.Other() < dto2.Other()) {
                result = -1;
            } else if(dto.Other() > dto2.Other()) {
                result = 1;
            } else {
                result = 0;
            }
        } 

return result

}

Then I will use the following way in my service:

Collections.sort(out,new MyComparator());
\$\endgroup\$
3
\$\begingroup\$

There are a number of issues with your implementation:

  1. Use Comparator<Dto>, not Comparator<Object>. Then there is no reason for check for instanceof. And the compiler will prevent you from trying to sort a collection of any other type of object.

  2. Relational operators don't work for strings (which getName() obviously returns).

  3. Null checking is needed.

Following is an implementation that might actually work:

public class MyComparator implements Comparator<Dto> {
    @Override
    public int compare(final Dto address1, final Dto address2) {
        if (address1 == null && address2 == null) {
            return 0;
        }
        if (address1 == null && address2 != null) {
            return -1;
        }
        if (address1 != null && address2 == null) {
            return 1;
        }

        // here we know that both address1 and address2 are not null
        int result = 0;
        final String name1 = address1.getName();
        final String name2 = address2.getName();

        if (name1 == null && name2 == null) {
            result = 0;
        } else if (name1 == null && name2 != null) {
            result = -1;
        } else if (name1 != null && name2 == null) {
            result = 1;
        }
        else {
            result = name1.compareTo(name2);
        }

        //if the Names are the same then compare the Number
        if (result == 0) {
            if (address1.Number() < address2.Number()) {
                result = -1;
            } else if (address1.Number() > address2.Number()) {
                result = 1;
            } else {
                result = 0;
            }
        }

        //if the Numbers are the same then compare the Other Field
        if (result == 0) {
            if (address1.Other() < address2.Other()) {
                result = -1;
            } else if (address1.Other() > address2.Other()) {
                result = 1;
            } else {
                result = 0;
            }
        }

        return result;
}

This assumes that Number() and Other() return some sort of base type (e.g., int).

Another possibility, if you have control over the Dto class, is to have Dto implement Comparator<Dto>, then put the compare method in that class. Then you can simply do

Collections.sort(out);

assuming out is a collection of Dto objects.

\$\endgroup\$
  • \$\begingroup\$ if Dto.getName() is guaranteed to not be null (by contract) then you don't need to check it for null. \$\endgroup\$ – ratchet freak Dec 10 '14 at 23:09
3
\$\begingroup\$

The shortest way to do this is using the Java 8 Comparator building api :

Comparator<Dto> myComparator = Comparator.comparing(Dto::getName)
    .thenComparing(Dto::Number)
    .thenComparing(Dto::Other);

And this is even a lot more readable.

Btw : your method names do not follow Java coding conventions, and the posted code doesn't actually compile.

\$\endgroup\$
1
\$\begingroup\$

This is what I have done but I do not know if this is the correct answer

The idea is perfectly fine, the implementation less so.

if (dto instanceof Dto) {
    final Dto dto = (Dto) address;

This makes no sense and can't compile. Such instanceof checks are needed in equals but not on compare.

        final Dto dto = (Dto) address;
        final Dto dto2 = (Dto) address2;

Switch on warnings, all the casts are unnecessary.

       if(dto.getName() < dto2.getName()) {

Have you numbered everyone? Otherwise this can't compile.

//if the Names are the same then compare the Number
        if (result == 0) {
            if(dto.Number() < dto2.Number()) {
                result = -1;
            } else if(dto.Number() > dto2.Number()) {
                result = 1;
            } else {
                result = 0;
            }

This is OK, except for the spacing (missing spaces after // and if) and naming (this is not C#, method names start with lowercase).

An early return like

if (result == 0) return result;

would make it a bit clearer. However, you structured it well.


But there's a much simpler solution using Guava:

import com.google.common.collect.ComparisonChain;
...

public int compare(final Dto address1, final Dto address2) {
    return ComparisonChain.start()
        .compare(address1.getName(), address2.getName())
        .compare(address1.getNumber(), address2.getNumber())
        .compare(address1.getOther(), address2.getOther())
        .result();
}

Nice, isn't it (Guava makes a lot of things much easier)?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.