There are several issues with your code. While this::compareAttrValueCountEntry
would be easy to
fix by changing it to ContainingClassName::compareAttrValueCountEntry
, this method is unnecessary
as there are several factory methods like Map.Entry.comparingByKey
, Map.Entry.comparingByValue
,
Comparator.reversed
and Comparator.thenComparing
, which can be combined to achieve the same goal
This guards you from the errors made within compareAttrValueCountEntry
. It’s tempting to compare int
values by subtracting, but this is error prone as the difference between two int
values doesn’t always
fit into the int
range, so overflows can occur. Also, negating the result for reversing the order is
broken, as the value might be Integer.MIN_VALUE
, which has no positive counterpart, hence, negating it
will overflow back to Integer.MIN_VALUE
instead of changing the sign.
Instead of looping via forEach
to add to another map, you may use a cleaner stream operation producing
the map and you can simplify sorted(…).findFirst()
to min(…)
which in not only shorter, but a
potentially cheaper operation.
Putting it together, we get
Map<Integer, String> attrIdMaxValueMap =
attrIdAttrValueCountMap.entrySet().stream()
.filter(e -> !e.getValue().isEmpty())
.collect(Collectors.toMap(Map.Entry::getKey,
e -> e.getValue().entrySet().stream()
.min(Map.Entry.<String, Integer>comparingByValue().reversed()
.thenComparing(Map.Entry.comparingByKey())).get().getKey()));
Note that I prepended a filter
operation rejecting empty maps, which ensures that there will always be
a matching element, so there is no need to deal with ifPresent
or such alike. Instead, Optional.get
can be called unconditionally.
Since this method is called findIdMaxValue
, there might be a desire to reflect that by calling max
on the Stream instead of min
, wich is only a matter of which comparator to reverse:
Map<Integer, String> attrIdMaxValueMap =
attrIdAttrValueCountMap.entrySet().stream()
.filter(e -> !e.getValue().isEmpty())
.collect(Collectors.toMap(Map.Entry::getKey,
e -> e.getValue().entrySet().stream()
.max(Map.Entry.<String, Integer>comparingByValue()
.thenComparing(Map.Entry.comparingByKey(Comparator.reverseOrder())))
.get().getKey()));
Unfortunately, such constructs hit the limitations of the type inference, which requires us to either,
use nested constructs (like Map.Entry.comparingByKey(Comparator.reverseOrder())
instead of
Map.Entry.comparingByKey().reversed()
) or to insert explicit types, like with
Map.Entry.<String, Integer>comparingByValue()
. In the second variant, reversing the second comparator,
we are hitting the litimation twice…
In this specific case, there might be a point in creating the comparator only once, keeping it in a variable and reuse it within the stream operation:
Comparator<Map.Entry<String, Integer>> valueOrMinKey
= Map.Entry.<String, Integer>comparingByValue()
.thenComparing(Map.Entry.comparingByKey(Comparator.reverseOrder()));
Map<Integer, String> attrIdMaxValueMap =
attrIdAttrValueCountMap.entrySet().stream()
.filter(e -> !e.getValue().isEmpty())
.collect(Collectors.toMap(Map.Entry::getKey,
e -> e.getValue().entrySet().stream().max(valueOrMinKey).get().getKey()));