Vavr: new Traversable.replace[All] method taking a predicate

[Sir4ur0n]

This scenario just popped up while coding today:
What if I have a Traversable (in my case, a List) and I want to replace all elements matching a predicate?

The cleanest solution I could come up with is:

// Replace all elements containing "an" with "replaced"
List<String> replaced = List.of("banana", "apple", "orange", "lemon")
    .map(fruit -> fruit.contains("an") ? "replaced" : fruit);
assertThat(replaced, is(List.of("replaced", "apple", "replaced", "lemon")));

I would expect to be able to write:

// Replace all elements containing "an" with "replaced"
List<String> replaced = List.of("banana", "apple", "orange", "lemon")
    .replaceAll(fruit -> fruit.contains("an"), "replaced");
assertThat(replaced, is(List.of("replaced", "apple", "replaced", "lemon")));

Have I missed a better writing?

Otherwise, the signatures I would expect are:

Traversable<T> replaceAll(Predicate<T> predicate, T newElement);
Traversable<T> replace(Predicate<T> predicate, T newElement);

[danieldietrich]

@Sir4ur0n Great suggestion, thank you. We should add the following, more general methods to Traversable:

Traversable<T> replace(Predicate<? super T> predicate, Function<? super T, ? extends T> mapper);

Traversable<T> replaceAll(Predicate<? super T> predicate, Function<? super T, ? extends T> mapper);

Note: I'm not sure if the existing methods break the Liskov Substitution Principle because SortedSet for example uses the underlying Comparator (instead of equals(obj)) to check object equality when calling replace/replaceAll.

• Java's TreeSet uses equals under the hood when calling remove(obj)
• Scala's TreeSet uses the underlying Comparator when calling the - method:

scala> import scala.collection.immutable._
import scala.collection.immutable._

scala> val set = TreeSet(1, 2, 3)((i,j) => i - j)
set: scala.collection.immutable.TreeSet[Int] = TreeSet(1, 2, 3)

scala> set - 1
res3: scala.collection.immutable.TreeSet[Int] = TreeSet(2, 3)

scala> val set = TreeSet(1, 2, 3)((i,j) => -1)
set: scala.collection.immutable.TreeSet[Int] = TreeSet(3, 2, 1)

scala> set - 1
res4: scala.collection.immutable.TreeSet[Int] = TreeSet(3, 2, 1)

However, Scala's TreeSet#- method isn't an overload, e.g. List does not have it (see this).

So, regarding LSP, I'm not sure if we also should unify that behavior or leave it this way.

I think is that we should leave it that way and clarify the behavior in the Javadoc. Element equality is defined by the collection type, depending on whether the collection isOrdered() == true, i.e. it has an underlying Comparator.

[nfekete]

Does it add any significant benefit in terms of either API syntax or performance compared to map(...)?

[danieldietrich]

@nfekete Good point! I've overseen that.

@Sir4ur0n Nándor is right, replaceAll does not add significant benefit compared to the existing map. The opposite is true: functions that take two or more function arguments are considered to be "bad practice" (can't find the original cite by Martin Odersky right now).

ts.replaceAll(t -> condition(t), t -> mapped(t));

ts.map(t -> condition(t) ? mapped(t) : t);

Expressing replace(Predicate, Function) is not that easy:

ts.find(t -> condition(t)).map(t -> ts.replace(t, mapped(t))).getOrElse(ts);

But without having a real-world use-case, we should not add additional API.

[Sir4ur0n]

Well, this issue was half-question (is there a better writing?), half-proposal.
I tend to dislike this ternary operator (I find it inexpressive), but this may just be me.
If it's just me, I guess we won't change the API ;-)

I also assume there's no better writing :'(

As for the _"functions that take two or more function arguments are considered to be "bad practice""_ part, would you have any source or explanation? I can't think of a reason this would be a bad practice. And I think there are many examples in Vavr API where this happens (e.g. io.vavr.Value#<K,V> Map<K,V> toMap(java.util.function.Function<? super T,? extends K> keyMapper, java.util.function.Function<? super T,? extends V> valueMapper) method).

[nfekete]

@Sir4ur0n I can't comment on the origin on that phrase but this is how I see it: in this particular case the two argument version (predicate + function) is better expressed with just a simple function (mapper) because in the former case, most of the time the predicate and the function are not independent of each other, rather, they are tightly related. They can also be elegantly compacted into one in the form predicate ? function(input) : input. The predicate + constant and predicate + supplier are just special mapper functions (0 argument supplier and constant function).

[CauchyPeano]

Hi @danieldietrich,
so I am not sure after your conversation if you want to have those methods to be defined in Traversable or not. For me it looks also that map method should be enough...

[danieldietrich]

@CauchyPeano yes, right

But without having a real-world use-case, we should not add additional API.

I will close the ticket!