Java streams map with sideeffect and collect or foreach and populate a result-list

Question

I have a piece of code that looks something like this.

I have read two contradicting(?) "rules" regarding this.

That .map should not have side effects
That .foreach should not update a mutable variable (so if I refactor to use foreach and populate a result list, then that breaks that) as mentioned in http://files.zeroturnaround.com/pdf/zt_java8_streams_cheat_sheet.pdf

How can I solve it so I use streams and still returns a list, or should I simply skip streams?

@Transactional
public Collection<Thing> save(Collection<Thing> things) {
    return things.stream().map(this::save).collect(Collectors.toList());
}

@Transactional
public Thing save(Thing thing) {
    // org.springframework.data.repository.CrudRepository.save 
    // Saves a given entity. Use the returned instance for further operations as the save operation might have changed the entity instance completely.
    Thing saved = thingRepo.save(thing);
    return saved;
}

Can you give exact source for rule regarding .foreach? Is it about variables of objects it is working on, or any kind of variable? (it is bit strange to say 'update mutable variable' - what would it mean to 'update immutable variable' ?) Anyway, I would probably suggest using for loop here, with explicit collection management, streams are not really saving you any work or giving more clarity here. — Artur Biesiadowski, Mar 22 '17 at 12:48
The key point is that you have to avoid *interference*. You can run into it, when the same `Thing` instance appears more than one time in the source collection, but I assume that you can preclude it due to your application logic. Then, it depends on how `thingRepo.save(…)` may alter `thingRepo`’s state. Generally, I’d consider not using a stream before resorting to populating a list via `forEach`, as the latter won’t result in simpler nor cleaner code than the `for` loop. — Holger, Mar 22 '17 at 15:52

score 4 · Accepted Answer · edited May 23 '17 at 12:32

4

Doesn't that paper say shared mutable state? In your case if you declare the list inside the method and then use forEach, everything is fine. The second answer here mentions exactly what you are trying to do.

edited May 23 '17 at 12:32

Community

1
1

answered Mar 22 '17 at 15:09

Eugene

117,005
15
201
306

Was my thinking to, but not sure what they meant by 'shared' since it was not obvious by the example. Not sure that you want shared mutable state at all in general if possible :) Second answer is exactly what I as after (sans the order) – Viktor Mellgren Mar 22 '17 at 15:50

score 1 · Answer 2 · edited Sep 02 '20 at 14:35

1

There is little to no reason to collect a entirely new List if you don't mutate it at all. Besides that your use case is basically iterating over every element in a collection and save that which could simply be achieved by using a for-each.

If for some reason thingRepo.save(thing) mutates the object you can still return the same collection, but at this point it's a hidden mutation which is not clearly visible at all since thingRepo.save(thing) does not suggest that.

edited Sep 02 '20 at 14:35

Viktor Mellgren

4,318
3
42
75

answered Mar 22 '17 at 12:47

Murat Karagöz

35,401
16
78
107

Save is mutating object, Saves a given entity. Use the returned instance for further operations as the save operation might have changed the entity instance completely. – Viktor Mellgren Mar 22 '17 at 12:52
1

@ViktorMellgren Then it's perfectly fine to return an entire new Collection. But this really depends on if you really need to have two collections with previous and current value of objects. – Murat Karagöz Mar 22 '17 at 12:56
I do :) Need the id that was set – Viktor Mellgren Mar 22 '17 at 13:13

Java streams map with sideeffect and collect or foreach and populate a result-list

2 Answers2