Reputation: 9724
Given the following key...
private class PositionKey {
String assetId;
String currencyId;
}
... I need to create a Map<PositionKey, BigDecimal>
, where if a given key does not exist, then a new entry shall be added, otherwise, if a given key already exists, then its values shall be increased. Here below is my attempt:
public Map<PositionKey, BigDecimal> getMap(final Long portfolioId) {
final Map<PositionKey, BigDecimal> map = new HashMap<>();
// getPositions() returns all the positions associated with the given portfolio
return getPositions(portfolioId).stream()
.collect(
toMap(
position ->
new PositionKey(
position.getAssetId(),
position.geCurrencyId()),
position -> position.geValue(),
(newValue, oldValue) -> oldValue != null ? oldValue.add(newValue) : newValue,
() -> map));
}
What I dislike in my implementation above, is the fact that I create the Map
before streaming the positions. Is there a better way to implement this? Thank you very much.
Upvotes: 0
Views: 489
Reputation: 28978
In case if you have no specific requirements regarding the Map
implementation, there's no need to use a version of Collector toMap()
which expects a mapFactory.
Instead, use three-args version, which expects only functions mapping keys/values and resolving duplicates, namely: keyMapper, valueMapper and a mergeFunction toMap(keyMapper, valueMapper, mergeFunction)
.
Also, you have messed with the order of arguments in the mergeFunction. The previously associated Value comes first, i.e. oldValue
then newValue
(not the opposite, although in this case it has no impact on the result since mergeFunction only performs addition of BigDecimal
values). And as @Holger has pointed out in the comments, there's no need to perform null-check unless valueMapper function can produce null
.
(oldValue, newValue) -> oldValue.add(newValue)
Which can be translated into a Method reference:
BigDecimal::add
That's how collect()
operation might look like in this case:
.collect(Collectors.toMap(
position -> new PositionKey(
position.getAssetId(),
position.geCurrencyId()
),
Position::geValue,
BigDecimal::add
))
And you would be provided with the default general purpose implementation of the Map
interface by default, which is for now HashMap
(if there would be another general purpose implementation which come as a recommended replacement of HashMap
you'll get it for free without changing the code). Hence, providing HashMap::new
as a mapFactory is frankly pointless.
A use case for the four-args version of toMap()
expecting a mapFactory would be a situation when you need a special purpose Map
implementation, like EnumMap
, TreeMap
, LinkedHashMap
, etc.
Upvotes: 1
Reputation: 716
simple! replace () -> map
with either () -> new HashMap<>()
or HashMap::new
Upvotes: 2