76

What I want to do is shown below in 2 stream calls. I want to split a collection into 2 new collections based on some condition. Ideally I want to do it in 1. I've seen conditions used for the .map function of streams, but couldn't find anything for the forEach. What is the best way to achieve what I want?

    animalMap.entrySet().stream()
            .filter(pair-> pair.getValue() != null)
            .forEach(pair-> myMap.put(pair.getKey(), pair.getValue()));

    animalMap.entrySet().stream()
            .filter(pair-> pair.getValue() == null)
            .forEach(pair-> myList.add(pair.getKey()));
Alex Shesterov
  • 26,085
  • 12
  • 82
  • 103
user3768533
  • 1,317
  • 2
  • 13
  • 21
  • 3
    Seems like a situation where streams aren't actually doing you any favors. It's just hiding control flow syntax with API in a way that turns out to be awkward and your `forEach` lambda is stateful. – Radiodef Jun 24 '16 at 20:21

4 Answers4

118

Just put the condition into the lambda itself, e.g.

animalMap.entrySet().stream()
        .forEach(
                pair -> {
                    if (pair.getValue() != null) {
                        myMap.put(pair.getKey(), pair.getValue());
                    } else {
                        myList.add(pair.getKey());
                    }
                }
        );

Of course, this assumes that both collections (myMap and myList) are declared and initialized prior to the above piece of code.


Update: using Map.forEach makes the code shorter, plus more efficient and readable, as Jorn Vernee kindly suggested:

    animalMap.forEach(
            (key, value) -> {
                if (value != null) {
                    myMap.put(key, value);
                } else {
                    myList.add(key);
                }
            }
    );
Community
  • 1
  • 1
Alex Shesterov
  • 26,085
  • 12
  • 82
  • 103
  • 7
    You could use ```Map.forEach``` instead, it would be a little more concise. – Jorn Vernee Jun 24 '16 at 19:59
  • 1
    You can also use braces `{ ... }` in the lambda expression, if it's more than a simple ternary can handle. – dcsohl Jun 24 '16 at 20:16
  • 1
    Thanks for your concise reply :) Hm, I'm getting "Bad return type in lambda expression: Serializable & Comparable extends Serializable & Comparable>> cannot be converted to void. – user3768533 Jun 24 '16 at 23:31
  • 1
    @user3768533, updated the answer to fix this problem. My bad, sorry for not being careful from the first time. Now compiled and tested the code. The reason for the compilation failure was that ternary operator is a wrong choice if it's about plain code flow (rather than an expression). [Here the relevant SO question on this topic.](http://stackoverflow.com/questions/9450864/java-ternary-with-no-return-for-method-calling) – Alex Shesterov Jun 25 '16 at 23:19
  • @AlexShesterov your updated code will throw NullPointerException if animalMap is null. so handle that. – Shivang Agarwal Apr 11 '18 at 10:09
  • 2
    @ShivangAgarwal, this is out of scope of this question, and also really trivial. There's no point to pollute answers with null-checks. Note that `myMap` and `myList` could be null as well. – Alex Shesterov Apr 12 '18 at 10:39
  • This is a good solution. But this is also a typical case of Shared Mutability. What if this is run in parallel? This will simply take us into an unknown outcomes. – NiNa Jan 08 '19 at 17:31
  • How to use ternary operator to check `animalMap` is null ? – Ashutosh Chamoli Jan 30 '20 at 07:38
18

In most cases, when you find yourself using forEach on a Stream, you should rethink whether you are using the right tool for your job or whether you are using it the right way.

Generally, you should look for an appropriate terminal operation doing what you want to achieve or for an appropriate Collector. Now, there are Collectors for producing Maps and Lists, but no out of-the-box collector for combining two different collectors, based on a predicate.

Now, this answer contains a collector for combining two collectors. Using this collector, you can achieve the task as

Pair<Map<KeyType, Animal>, List<KeyType>> pair = animalMap.entrySet().stream()
    .collect(conditional(entry -> entry.getValue() != null,
            Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue),
            Collectors.mapping(Map.Entry::getKey, Collectors.toList()) ));
Map<KeyType,Animal> myMap = pair.a;
List<KeyType> myList = pair.b;

But maybe, you can solve this specific task in a simpler way. One of you results matches the input type; it’s the same map just stripped off the entries which map to null. If your original map is mutable and you don’t need it afterwards, you can just collect the list and remove these keys from the original map as they are mutually exclusive:

List<KeyType> myList=animalMap.entrySet().stream()
    .filter(pair -> pair.getValue() == null)
    .map(Map.Entry::getKey)
    .collect(Collectors.toList());

animalMap.keySet().removeAll(myList);

Note that you can remove mappings to null even without having the list of the other keys:

animalMap.values().removeIf(Objects::isNull);

or

animalMap.values().removeAll(Collections.singleton(null));

If you can’t (or don’t want to) modify the original map, there is still a solution without a custom collector. As hinted in Alexis C.’s answer, partitioningBy is going into the right direction, but you may simplify it:

Map<Boolean,Map<KeyType,Animal>> tmp = animalMap.entrySet().stream()
    .collect(Collectors.partitioningBy(pair -> pair.getValue() != null,
                 Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
Map<KeyType,Animal> myMap = tmp.get(true);
List<KeyType> myList = new ArrayList<>(tmp.get(false).keySet());

The bottom line is, don’t forget about ordinary Collection operations, you don’t have to do everything with the new Stream API.

Community
  • 1
  • 1
Holger
  • 285,553
  • 42
  • 434
  • 765
  • Holger, don't you agree that your solution is definitely less readable than the accepted one ? – Marco Altieri Jun 05 '17 at 12:26
  • 6
    @Marco Altieri: that depends on the actual question. The question actually asked about the *Stream API*, which the accepted answer doesn’t really answer, as in the end, `forEach` is just an alternative syntax for a `for` loop. E.g., the `Map.forEach(…)` variant can’t be run in parallel, the `entrySet().stream().forEach(…)` variant will break awfully, when being run in parallel. When you want to use the Stream API and understand, how to use it correctly, you have to go with Alexis C’s answer or mine. Once you understood, it won’t look unreadable to you… – Holger Jun 06 '17 at 08:28
  • `when you find yourself using forEach on a Stream, you should rethink whether you are using the right tool for your job ` This is a strange statement. A typical example of a Collection Stream is a `forEach` or a `filter`. – gene b. Oct 31 '19 at 15:30
  • @geneb. no it isn’t. Typical cases are queries with *a result* and `forEach` is the one terminal action that doesn’t produce a result. Most attempts to solve a problem with `forEach` rather than one of the other terminal operations are loops in disguise and would work better with a loop. In example code (like on Stackoverflow) or testing code, you may find forEach with print statements, but serious application code doesn’t contain print statements. – Holger Oct 31 '19 at 15:52
12

The problem by using stream().forEach(..) with a call to add or put inside the forEach (so you mutate the external myMap or myList instance) is that you can run easily into concurrency issues if someone turns the stream in parallel and the collection you are modifying is not thread safe.

One approach you can take is to first partition the entries in the original map. Once you have that, grab the corresponding list of entries and collect them in the appropriate map and list.

Map<Boolean, List<Map.Entry<K, V>>> partitions =
    animalMap.entrySet()
             .stream()
             .collect(partitioningBy(e -> e.getValue() == null));

Map<K, V> myMap = 
    partitions.get(false)
              .stream()
              .collect(toMap(Map.Entry::getKey, Map.Entry::getValue));

List<K> myList =
    partitions.get(true)
              .stream()
              .map(Map.Entry::getKey) 
              .collect(toList());

... or if you want to do it in one pass, implement a custom collector (assuming a Tuple2<E1, E2> class exists, you can create your own), e.g:

public static <K,V> Collector<Map.Entry<K, V>, ?, Tuple2<Map<K, V>, List<K>>> customCollector() {
    return Collector.of(
            () -> new Tuple2<>(new HashMap<>(), new ArrayList<>()),
            (pair, entry) -> {
                if(entry.getValue() == null) {
                    pair._2.add(entry.getKey());
                } else {
                    pair._1.put(entry.getKey(), entry.getValue());
                }
            },
            (p1, p2) -> {
                p1._1.putAll(p2._1);
                p1._2.addAll(p2._2);
                return p1;
            });
}

with its usage:

Tuple2<Map<K, V>, List<K>> pair = 
    animalMap.entrySet().parallelStream().collect(customCollector());

You can tune it more if you want, for example by providing a predicate as parameter.

Alexis C.
  • 91,686
  • 21
  • 171
  • 177
9

I think it's possible in Java 9:

animalMap.entrySet().stream()
                .forEach(
                        pair -> Optional.ofNullable(pair.getValue())
                                .ifPresentOrElse(v -> myMap.put(pair.getKey(), v), v -> myList.add(pair.getKey())))
                );

Need the ifPresentOrElse for it to work though. (I think a for loop looks better.)

user3337629
  • 181
  • 3
  • 9