57

Suppose I have a stream of Things and I want to "enrich" them mid stream, I can use peek() to do this, eg:

streamOfThings.peek(this::thingMutator).forEach(this::someConsumer);

Assume that mutating the Things at this point in the code is correct behaviour - for example, the thingMutator method may set the "lastProcessed" field to the current time.

However, peek() in most contexts means "look, but don't touch".

Is using peek() to mutate stream elements an antipattern or ill-advised?

Edit:

The alternative, more conventional, approach would be to convert the consumer:

private void thingMutator(Thing thing) {
    thing.setLastProcessed(System.currentTimeMillis());
}

to a function that returns the parameter:

private Thing thingMutator(Thing thing) {
    thing.setLastProcessed(currentTimeMillis());
    return thing;
}

and use map() instead:

stream.map(this::thingMutator)...

But that introduces perfunctory code (the return) and I'm not convinced it's clearer, because you know peek() returns the same object, but with map() it's not even clear at a glance that it's the same class of object.

Further, with peek() you can have a lambda that mutates, but with map() you have to build a train wreck. Compare:

stream.peek(t -> t.setLastProcessed(currentTimeMillis())).forEach(...)
stream.map(t -> {t.setLastProcessed(currentTimeMillis()); return t;}).forEach(...)

I think the peek() version is clearer, and the lambda is clearly mutating, so there's no "mysterious" side effect. Similarly, if a method reference is used and the name of the method clearly implied mutation, that too is clear and obvious.

On a personal note, I don't shy away from using peek() to mutate - I find it very convenient.

Community
  • 1
Bohemian
  • 1,956
  • 2
  • 17
  • 24
  • 1
    Related: [Is it an antipattern, modifying an incoming parameter?](http://programmers.stackexchange.com/q/245767/40980) –  Feb 01 '16 at 21:55
  • 1
    Have you used `peek` with a stream that generates its elements dynamically? Does it still work, or are the changes lost? Modifying the elements of a stream sounds unreliable to me. – Sebastian Redl Feb 02 '16 at 09:26
  • 1
    @SebastianRedl I have found it 100% reliable. I first used it to collect during processing: `List list; things.stream().peek(list::add).forEach(...);` very handy. Lately. I've used it to add info for publishing: `Map timestamps = ...; return things.stream().peek(t -> t.setTimestamp(timestamp.get(t))).collect(toList());`. I know there are other ways of doing this example, but I am greatly over-simplifying here. Using `peek()` produces more compact, and more elegant code IMHO. Readability aside, this question is really about what you have raised; is it safe/reliable? – Bohemian Feb 03 '16 at 00:15
  • 3
    The question [In Java streams is peek really only for debugging?](http://stackoverflow.com/questions/33635717/in-java-streams-is-peek-really-only-for-debugging) may be interesting as well. – Vincent Savard May 12 '17 at 17:12
  • @Bohemian. Are you still practicing this approach with `peek`? I have similar question on stackoverflow and hope you could look at it and give your feedback. Thanks. https://stackoverflow.com/questions/47356992/possible-side-effect-of-stream-peek-changing-state-and-why-not-use-it-like-this?noredirect=1#comment81667659_47356992 – tsolakp Nov 17 '17 at 20:16
  • @tsolakp yes, I fearlessly use `peek()` whenever convenient for me to do so. Its use (and an answer to your SO question), is a matter of opinion/style/courage. – Bohemian Nov 17 '17 at 21:38
  • mutating the contents of a stream seems icky to me regardless of whether you use peek – jk. Apr 24 '20 at 11:54

3 Answers3

36

You are correct, "peek" in the English sense of the word means "look, but do not touch."

However the JavaDoc states:

peek

Stream peek(Consumer action)

Returns a stream consisting of the elements of this stream, additionally performing the provided action on each element as elements are consumed from the resulting stream.

Key words: "performing ... action" and "consumed". The JavaDoc is very clear that we should expect peek to have the ability to modify the stream.

However the JavaDoc also states:

API Note:

This method exists mainly to support debugging, where you want to see the elements as they flow past a certain point in a pipeline

This indicates that it is intended more for observing, e.g. logging elements in the stream.

What I take from all of this is that we can perform actions using the elements in the stream, but should avoid mutating elements in the stream. For example, go ahead and call methods on the objects, but try to avoid mutating operations on them.

At the very least, I would add a brief comment to your code along these lines:

// Note: this peek() call will modify the Things in the stream.
streamOfThings.peek(this::thingMutator).forEach(this::someConsumer);

Opinions differ on the usefulness of such comments, but I would use such a comment in this case.

  • 2
    Why do you need a comment if the method name clearly signals a mutation, eg `thingMutator`, or more concrete `resetLastProcessed` etc. Unless there's a compelling reason, the need comments such as your suggestion typically indicate poor choices of variable and/or method names. If good names are chosen, isn't that enough? Or are you saying that despite a good name, anything in `peek()` is like a blind spot that most programmers would "skim" (visually skip)? Also, "mainly for debugging" isn't the same thing as "only for debugging" - what usages other than the "mainly" ones were intended? – Bohemian Feb 02 '16 at 00:46
  • @Bohemian I would explain it mainly because the method name is non-intuitive. And I do not work for Oracle. I am not on their Java team. I have no idea what they intended. –  Feb 02 '16 at 04:09
  • 2
    @Bohemian because when looking for what modifies the elements in a way I don't like I will stop reading the line at "peek" because "peek" doesn't change anything. Only later when all other places of code didn't contain the bug I'm chasing will I come back to this with, possibly armed with a debugger and maybe then go "omg, who put this misleading code there?!" – Frank Hopkins Jun 24 '19 at 17:11
  • @Bohemian in the exampe here where everything is in one line, one needs to read on anyway, but one might skip the content of "peek" and often a stream statement goes over multiple lines with one stream operation per line – Frank Hopkins Jun 24 '19 at 17:12
  • The javadoc provides useful insight in the method parameter documentation: "action - a [non-interfering](https://docs.oracle.com/javase/8/docs/api/java/util/stream/package-summary.html#NonInterference) action to perform on the elements as they are consumed from the stream". That link states: "For most data sources, preventing interference means ensuring that the data source is *not modified at all* during the execution of the stream pipeline." – pyb Jul 11 '22 at 01:20
1

It could be easily misinterpreted, so I would avoid using it like this. Probably the best option is to use a lambda function to merge the two actions you need into the forEach call. You may also want to consider returning a new object rather than mutating the existing one - it may be slightly less efficient, but it is likely to result in more readable code, and reduces the potential of accidentally using the modified list for another operation that should have received the original.

Jules
  • 17,614
  • 2
  • 33
  • 63
-3

The API note tells us that the method has been added mainly for actions such as debugging/logging/firing stats etc.

@apiNote This method exists mainly to support debugging, where you want * to see the elements as they flow past a certain point in a pipeline: * 

       {@code
     *     Stream.of("one", "two", "three", "four")
     *         .filter(e -> e.length() > 3)
     *         .peek(e -> System.out.println("Filtered value: " + e))
     *         .map(String::toUpperCase)
     *         .peek(e -> System.out.println("Mapped value: " + e))
     *         .collect(Collectors.toList());
     * }
javaProgrammer
  • 103