7
\$\begingroup\$

Using Java 8, I'd like to sequence my eithers using a Collector:

Sequencing a stream of eithers means for me:

  • If there's at least one left either inside the stream, I expect the Collector to return the first left either.
  • If the eithers in the stream are exclusively right, I expect the Collector to return a list of each element inside the right eithers. That list is wrapped in a right either.

For example:

// Produces Either.Right([1, 2])
Stream.of(Either.right("1"), Either.right("2"))
        .collect(sequence());

// Produces Either.Left(too bad)
Stream.<Either<String, Integer>>of(Either.right(1), Either.left("too bad"),
        Either.left("no"))
        .collect(sequence());

Thus, it's similar in spirit to Fugue's Eithers#sequenceRight with the difference that I want a Collector to do the work, not converting an Iterable of Eithers to a single Either.

I came up with a solution that works for me but, oh my, the type of that thing:

/**
 * Creates a {@link Collector} that produces the first left
 * {@link Either either} from a {@link Stream}
 * of {@link Either}s or all the right elements if there is
 * no left element in the {@link Stream}.
 * <p>
 * Note: This is untested for parallel streams.
 *
 * @param <L> the type of left elements in the {@link Either}s
 * @param <R> the type of right elements in the {@link Either}s
 * @return a {@link Collector} sequencing a {@link Stream} of {@link Either}s
 */
public static <L, R> Collector<Either<L, R>,
        List<Either<L, List<R>>>,
        Either<L, List<R>>> sequence() {

    Supplier<String> getErrorMsg =
            () -> "Couldn't get either although it is right";
    // The combiner combines partial results of the collector.
    // Collector demands that we use a mutable container for the either.
    // We chose a list for that.
    BinaryOperator<List<Either<L, List<R>>>> combiner =
            (firstEitherContainer, secondEitherContainer) -> {
                // When both eithers are right, we combine their lists
                if (firstEitherContainer.get(0).isRight() &&
                        secondEitherContainer.get(0).isRight()) {
                    firstEitherContainer.get(0).getOrError(getErrorMsg)
                            .addAll(secondEitherContainer.get(0)
                                    .getOrError(getErrorMsg));
                    return firstEitherContainer;
                    // We found a left either -> Return that,
                    // throwing away the accumulated right eithers
                } else if (firstEitherContainer.get(0).isRight() &&
                        secondEitherContainer.get(0).isLeft()) {
                    return secondEitherContainer;
                } else {
                    // The first either was already left, continue with that
                    return firstEitherContainer;
                }
            };
    // The accumulator merges the elements inside the stream.
    // Again, the Collector demands that we use a mutable container
    // for the either, which is our list.
    BiConsumer<List<Either<L, List<R>>>, Either<L, R>> accumulator =
            (acc, either) -> {
                // Both eithers are right -> Merge their lists
                if (acc.get(0).isRight() && either.isRight()) {
                    acc.get(0).forEach(rights ->
                            rights.add(either.getOrError(getErrorMsg)));
                }
                // We found the first left either ->
                // This either is now the accumulator
                else if (acc.get(0).isRight() && either.isLeft()) {
                    acc.set(0, Either.left(either.left().get()));
                }
                // The accumulator already contained a left either ->
                // Leave the accumulator as it is
            };
    // Get the either from the list
    Function<List<Either<L, List<R>>>, Either<L, List<R>>> finisher =
            eithers -> eithers.get(0);

    return Collector.of(
            // The supplier for the start element has to be a mutable container.
            // Also, the list inside the either must be mutable since we
            // add other values to it.
            () -> Arrays.asList(Either.right(new ArrayList<>())),
            accumulator,
            combiner,
            finisher);
}

I would love to get feedback on that sequence function, especially on how to achieve the same with less code and shorter type signatures.

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

I agree with the answer given by Stingy. Also note that with the collector implementation you are not short-circuiting, so with a stream of a billion elements, you will still process them all even when the first element encountered is a left.

That being said, I will comment on a thing or two for this implementation:

  • Users of your collector generally do not really care about the type of the accumulator, so you can wildcard the type instead:

    <L, R> Collector<Either<L, R>, ?, Either<L, List<R>>> sequence()
    
  • I would swap the checks in your combiner, it makes it more readable in my opinion:

    Either<L, List<R>> firstEither = firstEitherContainer.get(0);
    if (firstEither.isLeft()) {
        return firstEitherContainer;
    }
    Either<L, List<R>> secondEither = secondEitherContainer.get(0);
    if (secondEither.isLeft()) {
        return secondEitherContainer;
    }
    firstEither.getOrError(getErrorMsg).addAll(secondEither.getOrError(getErrorMsg));
    return firstEitherContainer;
    
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Thanks for your feedback Koekje! singletonList() is immutable and will throw an UnsupportedOperationException whereas asList() returns a mutable list which we need in the combiner. \$\endgroup\$ May 2, 2018 at 16:48
  • 1
    \$\begingroup\$ Oh right, my bad! I had forgotten that for a second it seems :) \$\endgroup\$
    – Koekje
    May 2, 2018 at 17:13
  • 1
    \$\begingroup\$ For my own projects I use a custom 'ByRef' class, I like how it shows its intended purpose, instead of an array/list. \$\endgroup\$
    – Koekje
    May 2, 2018 at 17:18
  • \$\begingroup\$ Correcting my previous comment, it's the accumulator not the combiner that mutates that list using set which would throw if the list were immutable. \$\endgroup\$ May 2, 2018 at 17:31
5
\$\begingroup\$

You are horrified by "the type of that thing" because you are using stream operations for something they were not meant for. The purpose of streams is to perform pre-defined operations on an arbitrary number of elements of a kind. However, you have a scenario where the operation you want to perform depends on the nature of the stream itself. I did not really inspect your code in detail, because, based on your description of what you want to do, my suggestion would be to use a different approach altogether:

It is possible to obtain an iterator over the elements of a stream by calling iterator() on a stream (which is a terminal operation, so the stream will be consumed after creating the iterator, which means that you can obtain the iterator only once). Using that iterator, you can simply accumulate the elements inside the encountered Eithers in a List as long as they are all right eithers. If you encounter a left either, you stop iterating and return this left either. If the iterator is exhausted before you encounter a left either, you just wrap the now fully populated list in a right either and return that either.

\$\endgroup\$
2
\$\begingroup\$

Don't expose implementation details

The second type argument A of the Collector<T, A, R> interface, which refers to the internal accumulation type, should not be exposed for the following reasons:

  • it's an implementation detail, i.e. it might be a subject to change;
  • it makes the Collector type look very noisy and difficult to comprehend;
  • users of the Collector are interested only in the type of the stream element T and in the resulting type R.

Take a look at the Collectors returned from the methods of java.util.stream.Collectors class, none of them exposes the internal accumulation type A.

The very first and easiest improvement you can apply is to replace monstrosity List<Either<L,List<R>>> with an unknown type ?

public static <L, R> Collector<Either<L, R>, ?, Either<L, List<R>>> sequence()

All of a sudden, the return type fits into one line and code still compiles.

Don't comment - Refactor

Usage of inline comments is a smell.

Strive for the code which explains itself. The code should reveal its intention, as one of the principles of Extreme programming says.

Introducing an Abstraction

Method sequence() bears too much of a cognitive load.

But don't rush to attack the complexity, first, try to understand what kind of complexity you're dialing with. Whether it's an accidental complexity, or the inherent complexity of the task at hand.

Reviewing the functions, you'll notice that get(0) and set(0, ...) calls are making a lot of noise, which obstructs comprehending the logic of the Collector. Also, the accumulation type itself List<Either<L,List<R>>>, an Either having a List<R> as a value and wrapped with another list, might be confusing for the reader of the code.

List is an understandable choice for the mutable container, but it is not tailored for this task and adds accidental complexity.

Hence, my advice is to implement this Collector on top of a custom accumulation type. We can define it as a nested class hidden from the users of the Collector.

private static class EitherAccumulator<L, R> {
    private static final Supplier<String> MESSAGE_SUPPLIER =
        () -> "Couldn't get either although it is right";

    private Either<L, List<R>> acc;
    
    public EitherAccumulator(Either<L, List<R>> initial) {
        this.acc = initial;
    }
    
    public static <L, R> EitherAccumulator<L, R> emptyRight() {
        return new EitherAccumulator<>(Either.right(new ArrayList<>()));
    }
    
    public void accumulate(Either<? extends L, ? extends R> next) {
        if (acc.isLeft()) return;
        
        if (next.isLeft()) {
            acc = Either.left(next.left().get());
        } else {
            acc.forEach(rs -> rs.add(next.getOrError(MESSAGE_SUPPLIER)));
        }
    }
    
    public EitherAccumulator<L, R> combine(EitherAccumulator<L, R> other) {
        if (acc.isRight() && other.acc.isRight()) {
            acc.forEach(rs -> rs.addAll(other.acc.getOrError(MESSAGE_SUPPLIER)));
            return this;
        }
        
        return acc.isLeft() ? this : other;
    }
    
    public Either<L, List<R>> toEither() {
        return acc;
    }
}

Pay attention to the accumulator method. Its first condition performs an early return in order to simplify the subsequent conditions.

Guard clause is a common tool for reducing complexity of the conditional logic.

The Collector implementation will look like this:

public static <L, R> Collector<Either<L, R>, ?, Either<L, List<R>>> sequence() {
    return Collector.of(
        EitherAccumulator::<L, R>emptyRight,
        EitherAccumulator::accumulate,
        EitherAccumulator::combine,
        EitherAccumulator::toEither
    );
}

So-called type witness <L, R> in the supplier is required because method emptyRight() doesn't receive any parameters from which the compiler can properly infer the accumulation type.

Lack of short-circuiting

You need to be mindful that Stream.collect is not a short-circuit operation and will process all stream elements, contrary to some other terminal operations like Stream.anyMatch which may not evaluate its predicate on all elements.

Does it mean the idea of a Collector implementation performing an operation that can exit earlier if implemented in the imperative style has no right to exist?

If you have, let's say, a fixed-size list of twenty supplier functions producing an Either, then the potential overhead of processing them using a Stream is negligible. And there's no reason not to go fully functional, by don't forget to document properly collector's behavior.

For readers unfamiliar with Category theory and Functional programming, Either represents a functional approach to error-handling, without the use of exceptions. If you choose to use such functional constructs in your code, you're expected to employ a declarative programming model, rather than mixing them with imperative constructs like while-loops and iterators.

On the other hand, if this method is supposed to be a part of a public library, then it's a questionable addition and I would probably advise against introducing it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.