CODEWITHSUNDEEP
javalambdajava-stream
GhostCat
GhostCat

Reputation: 140427

How to remove multiple elements from Set/Map AND knowing which ones were removed?

I have a method that has to remove any element listed in a (small) Set<K> keysToRemove from some (potentially large) Map<K,V> from. But removeAll() doesn't do, as I need to return all keys that were actually removed, since the map might or might not contain keys that require removal.

Old-school code is straight forward:

public Set<K> removeEntries(Map<K, V> from) {
    Set<K> fromKeys = from.keySet();
    Set<K> removedKeys = new HashSet<>();
    for (K keyToRemove : keysToRemove) {
        if (fromKeys.contains(keyToRemove)) {
            fromKeys.remove(keyToRemove);
            removedKeys.add(keyToRemove);
        }
    }
    return removedKeys;
}

The same, written using streams:

Set<K> fromKeys = from.keySet();
return keysToRemove.stream()
        .filter(fromKeys::contains)
        .map(k -> {
            fromKeys.remove(k);
            return k;
        })
        .collect(Collectors.toSet());

I find that a bit more concise, but I also find that lambda too clunky.

Any suggestions how to achieve the same result in less clumsy ways?

Upvotes: 30

Views: 4637

Answers (6)

Oleksandr Pyrohov
Oleksandr Pyrohov

Reputation: 16216

More concise solution, but still with unwanted side effect in the filter call:

Set<K> removedKeys =
    keysToRemove.stream()
                .filter(fromKeys::remove)
                .collect(Collectors.toSet());

Set.remove already returns true if the set contained the specified element.

 P.S. In the end, I would probably stick with the "old-school code".

Upvotes: 13

Samuel Philipp
Samuel Philipp

Reputation: 11042

You can use this:

Set<K> removedKeys = keysToRemove.stream()
        .filter(from::containsKey)
        .collect(Collectors.toSet());
removedKeys.forEach(from::remove);

It's similar to Oleksandr's answer, but avoiding the side effect. But I would stick with that answer, if you are looking for performance.

Alternatively you could use Stream.peek() for the remove, but be careful with other side effects (see the comments). So I would not recommend that.

Set<K> removedKeys = keysToRemove.stream()
        .filter(from::containsKey)
        .peek(from::remove)
        .collect(Collectors.toSet());

Upvotes: 4

Naman
Naman

Reputation: 31878

To add another variant to the approaches, one could also partition the keys and return the required Set as:

public Set<K> removeEntries(Map<K, ?> from) {
    Map<Boolean, Set<K>> partitioned = keysToRemove.stream()
            .collect(Collectors.partitioningBy(k -> from.keySet().remove(k),
                    Collectors.toSet()));
    return partitioned.get(Boolean.TRUE);
}

Upvotes: 3

VGR
VGR

Reputation: 44328

I wouldn’t use Streams for this. I would take advantage of retainAll:

public Set<K> removeEntries(Map<K, V> from) {
    Set<K> matchingKeys = new HashSet<>(from.keySet());
    matchingKeys.retainAll(keysToRemove);

    from.keySet().removeAll(matchingKeys);

    return matchingKeys;
}

Upvotes: 5

Holger
Holger

Reputation: 298153

The “old-school code” should rather be

public Set<K> removeEntries(Map<K, ?> from) {
    Set<K> fromKeys = from.keySet(), removedKeys = new HashSet<>(keysToRemove);
    removedKeys.retainAll(fromKeys);
    fromKeys.removeAll(removedKeys);
    return removedKeys;
}

Since you said that keysToRemove is rather small, the copying overhead likely doesn’t matter. Otherwise, use the loop, but don’t do the hash lookup twice:

public Set<K> removeEntries(Map<K, ?> from) {
    Set<K> fromKeys = from.keySet();
    Set<K> removedKeys = new HashSet<>();
    for(K keyToRemove : keysToRemove)
        if(fromKeys.remove(keyToRemove)) removedKeys.add(keyToRemove);
    return removedKeys;
}

You can express the same logic as a stream as

public Set<K> removeEntries(Map<K, ?> from) {
    return keysToRemove.stream()
        .filter(from.keySet()::remove)
        .collect(Collectors.toSet());
}

but since this is a stateful filter, it is highly discouraged. A cleaner variant would be

public Set<K> removeEntries(Map<K, ?> from) {
    Set<K> result = keysToRemove.stream()
        .filter(from.keySet()::contains)
        .collect(Collectors.toSet());
    from.keySet().removeAll(result);
    return result;
}

and if you want to maximize the “streamy” usage, you can replace from.keySet().removeAll(result); with from.keySet().removeIf(result::contains), which is quiet expensive, as it is iterating over the larger map, or with result.forEach(from.keySet()::remove), which doesn’t have that disadvantage, but still, isn’t more readable than removeAll.

All in all, the “old-school code” is much better than that.

Upvotes: 26

MaanooAk
MaanooAk

Reputation: 2468

You can use the stream and the removeAll

Set<K> fromKeys = from.keySet();
Set<K> removedKeys = keysToRemove.stream()
    .filter(fromKeys::contains)
    .collect(Collectors.toSet());
fromKeys.removeAll(removedKeys);
return removedKeys;

Upvotes: 4

Related Questions