Vavr: Traversable::peek behaviour

[Abnaxos]

Javadoc:

Performs the given action on the first element if this is an eager implementation. Performs the given action on all elements (the first immediately, successive deferred), if this is a lazy implementation.

These are two radically different behaviours depending on the underlying implementation. I find that confusing.

Also, I need some way to process all elements in a Seq without changing the Seq and then continuing. Concrete use case: I have a Seq of all methods and want to apply multiple validations:

specialMethods = allMethods
  .filter(isSomeSpecialMethod())
  .peek(this::validateFoo)
  .peek(this::validateBar);

validateSomething() may print a compiler error, but in order to collect all possible compiler errors, it will continue. Logging all elements being processed is another possible use case for such a thing. I'll probably use a utility method for now as a workaround: filter(alwaysTrue(this::validateSomething))

While with Java streams peek() applies to all elements, the name "peek" somehow implies that it's only applied to the first element. Ruby has a method tap() for such use cases.

Still, the same interface method has very different behaviours depending on the underlying implementation, that's confusing and dangerous, IMHO.

[danieldietrich]

Well, it is a well-defined behavior - but I agree, it violates the LSP.

You know me, I don't really want to talk about naming (peek or tap or whatever). It is just a matter of definition or interpretation.

I'm interested in our options here. In my experience, it is hard to satisfy the LSP across a collection hierarchy. Let's consider a peek() method that is defined only on the first element (if present), no matter if the collection is lazy or not. There exist collections that do not have a defined order, like HashMap or HashSet. In particular, each time we call peek(), a different element may be returned.

peek() currently is defined on Value (the parent of Traversable). In order to satisfy LSP, we would need to move peek() to Seq.

But wait - a lazy Seq would still do nothing at all, if elements are present but not read.

So, why isn't it legit to define a different behavior for lazy values (and align this way to Java Stream)?

Note: We still are able to check, if a value is lazy (by calling isLazy()).

[danieldietrich]

It will stay as-is. It is a well-defined behavior.

[Abnaxos]

Sorry, I wasn't suggesting to change the behaviour of peek() –well, OK, I was, but that's not the primary issue. :) Bad issue reporting from my side.

My main point is that there's no API for a very common use case: offer each element to a consumer without any mapping or filtering. Like forEach() but as an intermediate operation instead of a terminal operation. As I wrote, I currently use this workaround:

public static Predicate<T> alwaysTrue(Consumer<T> consumer) {
  return e -> {
    consumer.accept(e);
    return true;
  }
}

mySeq.filter(alwaysTrue(e -> doSomethingWith(e)))

And I use this a lot. Unfortunately, it builds a new list every time, although I can tell in advance that it won't change anything. Naming proposals: tap(), touch(), offer() – I like touch().

If you like, I can file a new issue that's more clear.

Yes, there is some critic on the behaviour of peek(), but that's secondary.

[danieldietrich]

What would be a use-case other than performing side-effects that change the outer world? This is not a practice I want to further push than necessary.

A lightweight variant might be to operate on an iterator and finally convert to your collection of choice.

[danieldietrich]

Unfortunately, the forEach and methods can‘t return this because the are inherited from Iterable....

Maybe your solution is the best alternative. A touch would not make sense for lazily evaluated collections.

[Abnaxos]

A very common use case would be logging what's there.

One concrete use case where I stumbled upon this, is a compiler. The compiler should continue working when it encounters errors in order to find more errors. So, it looks at things, reports errors if it finds them, looks for more errors, transforms things further and so on. Very similar to logging, actually.

In a reactive context, you may want to use such a method in to push an object into a second processing stream. And that would make perfect sense for lazily evaluated collections. Although one might argue that Vavr isn't about reactive programming … :)

Sometimes, modifying the state of a mutable object may be what you want to do – although I completely agree that this is very bad practice. But Java isn't a native functional language and mutable objects are very common, so you may be forced to do such a thing when interacting with non-functional 3rd-party libraries. Using map() may be misleading in this situation, because normally, map() produces a new value. touch(e -> e.setFoo("foo")) may point out that it actually changes the state of the object at hand.

And an addition to my concern about the name: the method I'm missing exists in Java's Stream and it's called peek(). It took me quite a while to realise that Vavr's peek() is a completely different thing. Yes, I should have read the doc earlier, but can you really blame me for assuming, that a method with the same name in the same context does the same thing?

[danieldietrich]

I understand your concerns. It would be a duplication of forEach(). Please use iterator() or toStream().

forEach() is still a valid alternative if you don't stick to a fluent API.

UseTheRightCollectionForTheRightPurpose

[danieldietrich]

And an addition to my concern about the name: the method I'm missing exists in Java's Stream and it's called peek(). It took me quite a while to realise that Vavr's peek() is a completely different thing.

That's not true, see images above. It is exactly the same thing. However, you need to differentiate between _eager_ and _lazy_ evaluated collections. The implementation is canonical in a mathematical sense. There is nothing artificial baked on top of existing implementations, no balconies. I will not change that.