Refactoring to use CollectionUtils.mapValues#8059
Refactoring to use CollectionUtils.mapValues#8059clintropolis merged 10 commits intoapache:masterfrom
CollectionUtils.mapValues#8059Conversation
|
|
||
|
|
||
| @Override | ||
| @SuppressWarnings("SSBasedInspection") |
There was a problem hiding this comment.
When suppressing SSBasedInspection, please always add a comment -- what specifically you suppress (mapValues() in this case), and why -- it's not obvious for me in this case.
There was a problem hiding this comment.
ah, so i suppressed this, since the valueMapper function of CollectionUtils.mapValues() is not independent in this situation and needs map's key. May be there is a way around this, but was not clear to me, let me know if you have any suggestion so that i can remove the suppression.
| ); | ||
| versions = locks.entrySet().stream() | ||
| .collect(Collectors.toMap(Entry::getKey, entry -> entry.getValue().getVersion())); | ||
| versions = CollectionUtils.mapValues(locks, v -> v.getVersion()); |
There was a problem hiding this comment.
It's more readable to use method references in such cases because it makes evident the type of the value: versions = CollectionUtils.mapValues(locks, TaskLock::getVersion);. Or, it's better to choose more descriptive lambda param names (e. g. taskLock) than just v.
Same applies to many other places in this PR.
| <constraint name="k" within="" contains="" /> | ||
| <constraint name="__context__" target="true" within="" contains="" /> | ||
| </searchConfiguration> | ||
| <searchConfiguration name="Use CollectionUtils.mapValues(Map<k,v>, Function<v,v2>)" text="$x$.entrySet().stream().collect(Collectors.toMap(Entry::getKey, $y$))" recursive="true" caseInsensitive="true" type="JAVA"> |
There was a problem hiding this comment.
Please add patterns for mapKeys()
| public static <K, V, K2> Map<K2, V> mapKeys(Map<K, V> map, Function<K, K2> keyMapper) | ||
| { | ||
| final Map<K2, V> result = Maps.newHashMapWithExpectedSize(map.size()); | ||
| map.forEach((k, v) -> result.put(keyMapper.apply(k), v)); |
There was a problem hiding this comment.
I think it's better if this method fails with IllegalStateException on keys collisions if the key mapper is not a bijection, similar to ImmutableMap.Builder. This should also be reflected in the method's javadoc.
There was a problem hiding this comment.
Added a check for key collisions with keyMapper.
| map.forEach((k, v) -> result.put(keyMapper.apply(k), v)); | ||
| map.forEach((k, v) -> { | ||
| final K2 k2 = keyMapper.apply(k); | ||
| if (result.containsKey(k2)) { |
There was a problem hiding this comment.
Please make just one lookup operation for every key: putIfAbsent().
There was a problem hiding this comment.
@surekhasaharan the idea of this replacement was to get rid of containsKey() call, so that only one lookup operation (putIfAbsent() itself) is done on each iteration instead of two (containsKey() and put()):
if (result.putIfAbsent(k2, v) != null) {
throw new ISE("Conflicting key[%s] calculated via keyMapper for original key[%s]", k2, k);
}| * | ||
| * @throws ISE if key collisions occur while applying specified keyMapper | ||
| */ | ||
|
|
…segments-follow-up
3 participants
Description
This PR has some follow-ups changes left from #7595. Some minor doc updates and code updates to use
CollectionUtils.mapValuesandCollectionUtils.mapKeysutility methods.This PR has: