4

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

adbdkb
  • 1,897
  • 6
  • 37
  • 66
  • Just to be sure : your comparator is set to sort the keys of the first level map, not the second one. Is it your requirement ? – davidxxx Jul 12 '19 at 19:36
  • @davidxxx - Yes, the first level map. I think, when defining the comparator with the TreeMap, I need to indicate that, right? But not sure how to do that? – adbdkb Jul 12 '19 at 19:39

1 Answers1

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.

davidxxx
  • 125,838
  • 23
  • 214
  • 215
  • Hi, thanks for spotting that. But now I am running into other problem. The original map has more than 4000 rows whereas my new map has only 50 rows. _strValuesMap size 4444_ and _sortedMap size 50_ – adbdkb Jul 12 '19 at 20:02
  • Yeah - I knew I had to rewrite the comparator. Thanks for giving the pointers for those. But still not sure why the sizes of the two maps do not match – adbdkb Jul 12 '19 at 20:06
  • There must have been some other issue with my comparator. When I replaced it with the changes you had, it is now returning the map with 4444 rows and correctly sorted. – adbdkb Jul 12 '19 at 20:11
  • 1
    You are welcome. It is very likely. Verbose code produce often typing error not necessarily easy to spot. I updated with a Java 8 way. It may also be interesting. – davidxxx Jul 13 '19 at 07:05
  • Thanks for the alternate Java 8 way too. That also worked well and it is more compact and concise as well. – adbdkb Jul 15 '19 at 23:41