13

i have to arrays: arrA and arrB. arrA and arrB are Lists of objectss of diffrent types and add function converts objects A to objects B. I want to add each object from arrA to arrB and remove that object from arrA. Im trying to do this by stream:

arrA.stream().foreach(c -> {arrB.add(c); arrA.remove(c);});

when i execute this, two things are happening:

  1. not all objects are passed from arrA to arrB.
  2. after few iterations null pointer exception is thrown.

i gues it's because length of array is decreased after each remove() call and the counter of iterations is increased (only objects under odd indexes are passed to arrB)

Now i could solve this by copying array in one stream call and then remove objects in second stream call but this doesnt seem correct for me.

What would be proper solution to this problem?

EDIT. Additional information: in real implementation this list if previously filtered

arrA.stream().filter(some condition).foreach(c -> {arrB.add(c); arrA.remove(c);});

and its called few times to add elements meeting diffrent conditions to diffrent lists (arrC, arrD etc.) but each object can be only on one list

Akka Jaworek
  • 1,970
  • 4
  • 21
  • 47
  • 2
    Is it necessary you add and remove the objects one by one? You can clone the whole list and clear the other with only one method call, you know? – Hubert Grzeskowiak Sep 12 '16 at 14:14
  • What's the point of `arrA.remove` anyway? It's going to end up as empty, that's an invariant. – Marko Topolnik Sep 12 '16 at 14:15
  • added edit with brief explenation why i need to remove elements one by one – Akka Jaworek Sep 12 '16 at 14:20
  • 2
    Short and dirty: `listA.removeIf(c -> someCondition && listB.add(c));`, works, because `List.add` always returns `true`. – Holger Sep 12 '16 at 16:10
  • Possible duplicate of [Can Java 8 Streams operate on an item in a collection, and then remove it?](https://stackoverflow.com/questions/30041206/can-java-8-streams-operate-on-an-item-in-a-collection-and-then-remove-it) – AlikElzin-kilaka Mar 31 '18 at 12:02

5 Answers5

18

Streams are designed to be used in a more functional way, preferably treating your collections as immutable.

The non-streams way would be:

arrB.addAll(arrA);
arrA.clear();

However you might be using Streams so you can filter the input so it's more like:

arrB.addAll(arrA.stream().filter(x -> whatever).toList())

then remove from arrA (thanks to @Holgar for the comment).

arrA.removeIf(x -> whatever)

If your predicate is expensive, then you could partition:

Map<Boolean, XXX> lists = arrA.stream()
  .collect(Collectors.partitioningBy(x -> whatever));
arrA = lists.get(false);
arrB = lists.get(true);

or make a list of the changes:

List<XXX> toMove = arrA.stream().filter(x->whatever).toList();
arrA.removeAll(toMove);
arrB.addAll(toMove);
davidsheldon
  • 38,365
  • 4
  • 27
  • 28
  • +1 for your last "list of changes". This seems to get closest to what OP wants. Everything else seems to not directly do what OP wants or seems less efficient. – Martin Nyolt Sep 12 '16 at 14:31
  • 1
    @Martin Nyolt: actually, there is nothing worse than the “list of changes” approach. The time complexity of the `removeAll` will be n×m, up to quadratic if all elements match the filter. – Holger Sep 12 '16 at 16:03
  • 2
    The `partitioningBy` approach deserves my +1. As an addendum, if `arrA` is mutable (as the initial `remove` approach suggests), `arrA = arrA.stream().filter(x -> whatever).toList()` can be replaced with `arrA.removeIf(x -> !whatever);`… – Holger Sep 12 '16 at 16:05
  • `removeAll` being n×m: good point, didn't thought about that. Making it linear is surely possible, as the order is the same in both `toMove` and `arrA` - but that's probably not worth the effort compared to other approaches. – Martin Nyolt Sep 12 '16 at 16:12
7

As the others have mentioned, this is not possible with foreach - as it is impossible with the for (A a: arrA) loop to remove elements.

In my opinion, the cleanest solution is to use a plain for while with iterators - iterators allow you to remove elements while iterating (as long as the collection supports that).

Iterator<A> it = arrA.iterator()
while (it.hasNext()) {
    A a = it.next();
    if (!check(a))
        continue;
    arrB.add(a);
    it.remove();
}

This also saves you from copying/cloning arrA.

Martin Nyolt
  • 4,463
  • 3
  • 28
  • 36
  • 2
    I prefer this solution. I believe streams are good as long as I don't need to fight them to achieve what I need. – RealSkeptic Sep 12 '16 at 15:35
1

I don't think you can remove from arrA while you iterate over it.

You can get around this by wrapping it in a new ArrayList<>();

new ArrayList<>(arrA).stream().foreach(c -> {arrB.add(c); arrA.remove(c);});

Mike Samaras
  • 376
  • 2
  • 13
1

i guess it's because length of array is decreased after each remove() call and the counter of iterations is increased

Right. the for-each-loop is just like a normal for-loop, but easier to write and read. You can think of it as syntactic sugar. Internally it will either use an Iterator or array indices. The forEach method of streams is a more fancy version of it that allows parallel execution and functional coding style, but has its own drawbacks.

As with any indexed loop, removing an element while looping breaks the loop. Consider having three elements with indices 0, 1, and 2. When you remove element 0 in the first iteration, the list items will shift one up and the next iteration you'll have elements 0 (previously 1) and 1 (previously 2). Your loop variable now points to 1, so it skips the actually next item. When it gets to index 2 the loop you're working on only has one item left (you removed two), which throws an error because the index is out of bounds.

Possible solutions:

  • Use the List methods for cloning and clearing lists.
  • Do it with two loops if you really need to call the methods on each single item.
Community
  • 1
  • 1
Hubert Grzeskowiak
  • 15,137
  • 5
  • 57
  • 74
-1

You could just do Collections.addAll. Then when that's finished. just call clear() on arrA.

Jacob
  • 91
  • 1
  • 9