Comparing objects based on the values of multiple fields using Comparator

Question

How can I improve the comparator for objects by several fields? The comparator must sort user by last name or first name if last name doesn't exist. And if there are no last name and first name sort by username. But these users must be at the end of the list.

public static Comparator<UserConfigurationDto> BY_LASTNAME =  (u1, u2) -> {
        // if users have only username compare them
        if((u1.getLastName().isEmpty() && u1.getFirstName().isEmpty())
                && (u2.getLastName().isEmpty() && u2.getFirstName().isEmpty())){
            return u1.getUsername().compareToIgnoreCase(u2.getUsername());
        }
        //if user doesnt have firstName and LastName drop them at the end
        if(u1.getLastName().isEmpty() && u1.getFirstName().isEmpty()){
            return 1000000 + getWeight(u1.getUsername());
        }
        if(u2.getLastName().isEmpty() && u2.getFirstName().isEmpty()){
            return -1000000 + getWeight(u2.getUsername());
        }
      String s1 = u1.getLastName().isEmpty() ? u1.getFirstName() : u1.getLastName();
      String s2 = u2.getLastName().isEmpty() ? u2.getFirstName() : u2.getLastName();
      return s1.compareToIgnoreCase(s2);
    };
}
private static int getWeight(String s){
    return s.codePoints().sum();
}

Does anybody have an idea how to improve this? I try to use Comparator.comparing and Comparator.thenComparing but they produce an incorrect result

Answers already provided, but a potential additional improvement is for the Comparator to be in its own class instead of a lambda as part of some other class. The purpose of the separate class is to make it easy to unit test since it is not trivial. — Andrew S, May 31 '19 at 17:42

score 1 · Accepted Answer · answered May 31 '19 at 17:31

1) return 1000000 + getWeight(u1.getUsername()); and return -1000000 + getWeight(u2.getUsername()); are not required. return 1 and return -1 is clearer and produces the same result if you refer to the CompareTo() javadoc :

Compares this object with the specified object for order. Returns a negative integer, zero, or a positive integer as this object is less than, equal to, or greater than the specified object

2) You don't chain the field comparisons but you have 3 ways of sorting according to the state of the compared objects. So the fact that the code be a bit verbose to define each case is finally normal.
You could all the same reduce it with an extract method as you duplicate a lot of user.getLastName().isEmpty() invocations.

For example :

public static Comparator<UserConfigurationDto> BY_LASTNAME =  (u1, u2) -> {

        // first case
        if( u1.isLastAndFirstNameEmpty() && u2.isLastAndFirstNameEmpty()){
            return u1.getUsername().compareToIgnoreCase(u2.getUsername());
        }
        // second case
        if(u1.isLastAndFirstNameEmpty()){
            return 1;
        }
        else if(u2.isLastAndFirstNameEmpty()){
            return -1;
        }
        // third case
        String s1 = u1.getLastName().isEmpty() ? u1.getFirstName() : u1.getLastName();
        String s2 = u2.getLastName().isEmpty() ? u2.getFirstName() : u2.getLastName();
        return s1.compareToIgnoreCase(s2);
    };

score 1 · Answer 2 · answered May 31 '19 at 17:32

Where you're going wrong is trying to apply the idea of a "weight". When dealing with Comparators in Java, the only values that matter coming out of the function are 0, greater than 0, and less than 0.

Something like this seems to work:

public static final Comparator<UserConfgurationDto> BY_LASTNAME = ( u1, u2 ) ->
{
    boolean u1hasName = !u1.getLastName().isEmpty() || !u1.getFirstName().isEmpty();
    boolean u2hasName = !u2.getLastName().isEmpty() || !u2.getFirstName().isEmpty();

    if ( u1hasName && !u2hasName )
    {
        // u1 < u2
        return -1;
    }
    else if ( !u1hasName && u2hasName )
    {
        // u2 < u1
        return 1;
    }
    else if ( u1hasName && u2hasName )
    {
        String s1 = u1.getLastName().isEmpty() ? u1.getFirstName() : u1.getLastName();
        String s2 = u2.getLastName().isEmpty() ? u2.getFirstName() : u2.getLastName();

        return s1.compareToIgnoreCase( s2 );
    }
    else
    {
        return u1.getUsername().compareToIgnoreCase( u2.getUsername() );
    }
};

score 1 · Answer 3 · answered May 31 '19 at 17:40

I think you can use the Java 8 static comparing function that includes the key comparator to achieve this. String has a handy case insensitive comparator that you can use in a few places as well.

Comparator.comparing(
                (UserConfigurationDto u) -> u.getLastName().isEmpty()?u.getFirstName():u.getLastName(),
                ((s1, s2) -> {
                    if(s1.isEmpty()) {
                        return 1;
                    } else if(s2.isEmpty()) {
                        return -1;
                    }
                    else {
                        return String.CASE_INSENSITIVE_ORDER.compare(s1, s2);
                    }
                }))
                .thenComparing(UserConfigurationDto::getUsername, String.CASE_INSENSITIVE_ORDER);

Comparing objects based on the values of multiple fields using Comparator

3 Answers3