How to sort a Hashmap on Key based on Custom Comparator

Question

I am receiving a Hashmap for a common process that I need to sort on the Key using a custom comparator.

Below is what I have tried, but it does not seem to work - it is not sorting the keys.

The keys of the map are of the form long-string-short Example:( 11169-SW-1 / 11169-SW-2 / 11132-SH-2 / 11132-SH-7 / 11132-SH-1 ). The string compare doesn't work as I need it for the numeric part, so I have a custom comparator defined. I realise that the custom comparator code needs to be cleaned-up - e.g. multiple return statements need to be consolidated and othe clean-up needs to happen, but I can do that once I get it to work.

How can I do it?

Map<String, Map<String, String>> strValuesMap = Utilities.getDataMapFromDB();

Map<String, Map<String, String>> sortedMap = new TreeMap<>(new CustomComparator());
sortedMap.putAll(strValuesMap);
sortedMap.forEach((x, y) -> System.out.println("id " + x + "=" + y));

The custom comparator is defined as below

class CustomComparator implements Comparator<String> {

    @Override
    public int compare(String plId1, String plId2) {

        System.out.println("plId1 : " + plId1 + " plId2 " + plId2);
        String[] plId1Split = plId1.split("-");
        String[] plId2Split = plId2.split("-");
        int retValue = 0;
        if (!plId1Split[0].equalsIgnoreCase(plId2Split[0])) {
            Long seq1 = new Long(plId1Split[0]);
            Long seq2 = new Long(plId2Split[0]);
            retValue = seq1.compareTo(seq1);
        }
        if (retValue != 0) {
            return retValue;
        }
        if (!plId1Split[1].equalsIgnoreCase(plId2Split[1])) {
            retValue = plId1.compareTo(plId2);
        }
        if (retValue != 0) {
            return retValue;
        } else {
            Short seq1 = new Short(plId1Split[2]);
            Short seq2 = new Short(plId2Split[2]);
            retValue = seq1.compareTo(seq2);
            return retValue;
        }
    }
}

Thanks

Answer 1

This is not correct:

if (!plId1Split[1].equalsIgnoreCase(plId2Split[1])) {
    retValue = plId1.compareTo(plId2); // This part is wrong
}

In the conditional body, you should compare only the second part of the strings(plId1Split[1] with plId2Split[1]) while you compare the whole strings (plId1 with plId2).
So it means the last part of the strings (plId1Split[2] and plId2Split[2]) are not compared according to a numerical order but a lexicographical order.

So it should be :

if (!plId1Split[1].equalsIgnoreCase(plId2Split[1])) {
    retValue = plId1Split[1].compareTo(plId2Split[1]);
}

About the clarity of your comparator I think that some of your tests are not required.
For example you compare with equalsIgnoreCase() and you compare then with long comparator the same string converted to long :

if (!plId1Split[0].equalsIgnoreCase(plId2Split[0])) {
        Long seq1 = new Long(plId1Split[0]);
        Long seq2 = new Long(plId2Split[0]);
        retValue = seq1.compareTo(seq1);
} 

It not a great optimization, overall if the two comparisons are invoked and it also makes the code less readable.

Note also that you overuse number objects which here produce undesirable boxing operations (Long->long) that have a cost.
You could write something like :

class CustomComparator implements Comparator<String> {

    @Override
    public int compare(String plId1, String plId2) {

        String[] plId1Split = plId1.split("-");
        String[] plId2Split = plId2.split("-");

        int retValue = Long.compare(Long.parseLong(plId1Split[0]), 
                               Long.parseLong(plId2Split[0]));

        if (retValue != 0) {
            return retValue;
        }

        retValue = plId1Split[1].compareTo(plId2Split[1]);

        if (retValue != 0) {
            return retValue;
        }

        return Short.compare(Short.parseShort(plId1Split[2]), 
                       Short.parseShort(plId2Split[2]));

   }                    
}

---

As alternative you could use Java 8 Comparators by relying on a custom class representing the object to compare :

// private package if makes sense to make this class not visible outside that
class Identifier {
    private long part1;
    private String part2;
    private short part3;

    Identifier(String[] split) {
        this.part1 =  Long.parseLong(split[0]);
        this.part2 =  split[1];
        this.part3 =  Short.parseShort(split[2]);
    }

    long getPart1() {
        return part1;
    }    
    String getPart2() {
        return part2;
    }
    short getPart3() {
        return part3;
    }
}

And use public static <T, U> Comparator<T> comparing( Function<? super T, ? extends U> keyExtractor, Comparator<? super U> keyComparator)

that extracts a sort key to compare elements and applies a specified comparator for this sort key :

import static java.util.Comparator.comparing;
import static java.util.Comparator.comparingLong;

Comparator<String> comp =
        comparing(s -> new Identifier(s.split("-")),
                  comparingLong(Identifier::getPart1)
                 .thenComparing(Identifier::getPart2)
                 .thenComparingInt(Identifier::getPart3));

It exposes clearly the comparing/functional logic.

Note that since Java doesn't provide built-in structures for Tuple, we are required to introduce a custom class to hold data.
With tuple classes (coming from https://www.javatuples.org/ or any source code) the code would be still simpler.