Lambdas, multiple forEach with casting
Asked Answered
K

4

30

Need some help thinking in lambdas from my fellow StackOverflow luminaries.

Standard case of picking through a list of a list of a list to collect some children deep in a graph. What awesome ways could Lambdas help with this boilerplate?

public List<ContextInfo> list() {
    final List<ContextInfo> list = new ArrayList<ContextInfo>();
    final StandardServer server = getServer();

    for (final Service service : server.findServices()) {
        if (service.getContainer() instanceof Engine) {
            final Engine engine = (Engine) service.getContainer();
            for (final Container possibleHost : engine.findChildren()) {
                if (possibleHost instanceof Host) {
                    final Host host = (Host) possibleHost;
                    for (final Container possibleContext : host.findChildren()) {
                        if (possibleContext instanceof Context) {
                            final Context context = (Context) possibleContext;
                            // copy to another object -- not the important part
                            final ContextInfo info = new ContextInfo(context.getPath());
                            info.setThisPart(context.getThisPart());
                            info.setNotImportant(context.getNotImportant());
                            list.add(info);
                        }
                    }
                }
            }
        }
    }
    return list;
}

Note the list itself is going to the client as JSON, so don't focus on what is returned. Must be a few neat ways I can cut down the loops.

Interested to see what my fellow experts create. Multiple approaches encouraged.

EDIT

The findServices and the two findChildren methods return arrays

EDIT - BONUS CHALLENGE

The "not important part" did turn out to be important. I actually need to copy a value only available in the host instance. This seems to ruin all the beautiful examples. How would one carry state forward?

final ContextInfo info = new ContextInfo(context.getPath());
info.setHostname(host.getName()); // The Bonus Challenge
Khanate answered 22/8, 2014 at 3:52 Comment(2)
Challenge accepted.Conspecific
I accepted it firstBreaststroke
C
38

It's fairly deeply nested but it doesn't seem exceptionally difficult.

The first observation is that if a for-loop translates into a stream, nested for-loops can be "flattened" into a single stream using flatMap. This operation takes a single element and returns an arbitrary number elements in a stream. I looked up and found that StandardServer.findServices() returns an array of Service so we turn this into a stream using Arrays.stream(). (I make similar assumptions for Engine.findChildren() and Host.findChildren().

Next, the logic within each loop does an instanceof check and a cast. This can be modeled using streams as a filter operation to do the instanceof followed by a map operation that simply casts and returns the same reference. This is actually a no-op but it lets the static typing system convert a Stream<Container> to a Stream<Host> for example.

Applying these transformations to the nested loops, we get the following:

public List<ContextInfo> list() {
    final List<ContextInfo> list = new ArrayList<ContextInfo>();
    final StandardServer server = getServer();

    Arrays.stream(server.findServices())
        .filter(service -> service.getContainer() instanceof Engine)
        .map(service -> (Engine)service.getContainer())
        .flatMap(engine -> Arrays.stream(engine.findChildren()))
        .filter(possibleHost -> possibleHost instanceof Host)
        .map(possibleHost -> (Host)possibleHost)
        .flatMap(host -> Arrays.stream(host.findChildren()))
        .filter(possibleContext -> possibleContext instanceof Context)
        .map(possibleContext -> (Context)possibleContext)
        .forEach(context -> {
            // copy to another object -- not the important part
            final ContextInfo info = new ContextInfo(context.getPath());
            info.setThisPart(context.getThisPart());
            info.setNotImportant(context.getNotImportant());
            list.add(info);
        });
    return list;
}

But wait, there's more.

The final forEach operation is a slightly more complicated map operation that converts a Context into a ContextInfo. Furthermore, these are just collected into a List so we can use collectors to do this instead of creating and empty list up front and then populating it. Applying these refactorings results in the following:

public List<ContextInfo> list() {
    final StandardServer server = getServer();

    return Arrays.stream(server.findServices())
        .filter(service -> service.getContainer() instanceof Engine)
        .map(service -> (Engine)service.getContainer())
        .flatMap(engine -> Arrays.stream(engine.findChildren()))
        .filter(possibleHost -> possibleHost instanceof Host)
        .map(possibleHost -> (Host)possibleHost)
        .flatMap(host -> Arrays.stream(host.findChildren()))
        .filter(possibleContext -> possibleContext instanceof Context)
        .map(possibleContext -> (Context)possibleContext)
        .map(context -> {
            // copy to another object -- not the important part
            final ContextInfo info = new ContextInfo(context.getPath());
            info.setThisPart(context.getThisPart());
            info.setNotImportant(context.getNotImportant());
            return info;
        })
        .collect(Collectors.toList());
}

I usually try to avoid multi-line lambdas (such as in the final map operation) so I'd refactor it into a little helper method that takes a Context and returns a ContextInfo. This doesn't shorten the code at all, but I think it does make it clearer.

UPDATE

But wait, there's still more.

Let's extract the call to service.getContainer() into its own pipeline element:

    return Arrays.stream(server.findServices())
        .map(service -> service.getContainer())
        .filter(container -> container instanceof Engine)
        .map(container -> (Engine)container)
        .flatMap(engine -> Arrays.stream(engine.findChildren()))
        // ...

This exposes the repetition of filtering on instanceof followed by a mapping with a cast. This is done three times in total. It seems likely that other code is going to need to do similar things, so it would be nice to extract this bit of logic into a helper method. The problem is that filter can change the number of elements in the stream (dropping ones that don't match) but it can't change their types. And map can change the types of elements, but it can't change their number. Can something change both the number and types? Yes, it's our old friend flatMap again! So our helper method needs to take an element and return a stream of elements of a different type. That return stream will contain a single casted element (if it matches) or it will be empty (if it doesn't match). The helper function would look like this:

<T,U> Stream<U> toType(T t, Class<U> clazz) {
    if (clazz.isInstance(t)) {
        return Stream.of(clazz.cast(t));
    } else {
        return Stream.empty();
    }
}

(This is loosely based on C#'s OfType construct mentioned in some of the comments.)

While we're at it, let's extract a method to create a ContextInfo:

ContextInfo makeContextInfo(Context context) {
    // copy to another object -- not the important part
    final ContextInfo info = new ContextInfo(context.getPath());
    info.setThisPart(context.getThisPart());
    info.setNotImportant(context.getNotImportant());
    return info;
}

After these extractions, the pipeline looks like this:

    return Arrays.stream(server.findServices())
        .map(service -> service.getContainer())
        .flatMap(container -> toType(container, Engine.class))
        .flatMap(engine -> Arrays.stream(engine.findChildren()))
        .flatMap(possibleHost -> toType(possibleHost, Host.class))
        .flatMap(host -> Arrays.stream(host.findChildren()))
        .flatMap(possibleContext -> toType(possibleContext, Context.class))
        .map(this::makeContextInfo)
        .collect(Collectors.toList());

Nicer, I think, and we've removed the dreaded multi-line statement lambda.

UPDATE: BONUS CHALLENGE

Once again, flatMap is your friend. Take the tail of the stream and migrate it into the last flatMap before the tail. That way the host variable is still in scope, and you can pass it to a makeContextInfo helper method that's been modified to take host as well.

    return Arrays.stream(server.findServices())
        .map(service -> service.getContainer())
        .flatMap(container -> toType(container, Engine.class))
        .flatMap(engine -> Arrays.stream(engine.findChildren()))
        .flatMap(possibleHost -> toType(possibleHost, Host.class))
        .flatMap(host -> Arrays.stream(host.findChildren())
                               .flatMap(possibleContext -> toType(possibleContext, Context.class))
                               .map(ctx -> makeContextInfo(ctx, host)))
        .collect(Collectors.toList());
Conspecific answered 22/8, 2014 at 5:15 Comment(2)
You could build an API from this alone... amazing work.Advisee
Since you are designing toType for exactly the application here, you could rewrite it as <T,U> Function<T,Stream<U>> toType(Class<U> clazz) { return t -> { ... }; } so that it can be invoked using just .flatMap(toType(Engine.class)) without creating another anonymous lambda class.Schnook
E
26

This would be my version of your code using JDK 8 streams, method references, and lambda expressions:

server.findServices()
    .stream()
    .map(Service::getContainer)
    .filter(Engine.class::isInstance)
    .map(Engine.class::cast)
    .flatMap(engine -> Arrays.stream(engine.findChildren()))
    .filter(Host.class::isInstance)
    .map(Host.class::cast)
    .flatMap(host -> Arrays.stream(host.findChildren()))
    .filter(Context.class::isInstance)
    .map(Context.class::cast)
    .map(context -> {
        ContextInfo info = new ContextInfo(context.getPath());
        info.setThisPart(context.getThisPart());
        info.setNotImportant(context.getNotImportant());
        return info;
    })
    .collect(Collectors.toList());

In this approach, I replace your if-statements for filter predicates. Take into account that an instanceof check can be replaced with a Predicate<T>

Predicate<Object> isEngine = someObject -> someObject instanceof Engine;

which can also be expressed as

Predicate<Object> isEngine = Engine.class::isInstance

Similarly, your casts can be replaced by Function<T,R>.

Function<Object,Engine> castToEngine = someObject -> (Engine) someObject;

Which is pretty much the same as

Function<Object,Engine> castToEngine = Engine.class::cast;

And adding items manually to a list in the for loop can be replaced with a collector. In production code, the lambda that transforms a Context into a ContextInfo can (and should) be extracted into a separate method, and used as a method reference.

Encrust answered 22/8, 2014 at 5:7 Comment(9)
Mind. blown. . . . all. over. the. floor.Khanate
The two findChildren methods return arrays. How would you handle that?Khanate
I didn't. I may have missintepreted your code, the solution could be to map those to streams. Using something like Arrays::stream and possibly a flatmap. I don't know, this is just an idea on how to do it.Encrust
Not your fault on arrays, I didn't have that information in the questions when you answered. Inspiring use of PredicateKhanate
Interesting use of method references for Class::isInstance and Class::cast.Conspecific
from an FP perspective, mutating stuff and doing list.add() feels pretty wacky. From an OO perspective, while this example is clearly more readable than the OP's, it's still lagging far behind LINQ, where you would use OfType<T>() instead of that reflection thing.Dryer
@HighCore For more referential transparency, we could use a map lambda to create the context info and the terminal method would be collect(Collectors.toList()) which can then be used to either do the mutation of the original list or simply replace it. Unfortunately the Streams API is still light years away from LINQ. Maybe in another 4 or 5 years :-)Encrust
@HighCore: fixed. Mapping and using a collector (as I've just seen Edwin Dalorzo suggest) is the side-effect free way of doing that.Untimely
You need to use Stream.of to convert T[] array to Stream<T>.Bey
B
2

Solution to bonus challenge

Inspired by @EdwinDalorzo answer.

public List<ContextInfo> list() {
    final List<ContextInfo> list = new ArrayList<>();
    final StandardServer server = getServer();

    return server.findServices()
            .stream()
            .map(Service::getContainer)
            .filter(Engine.class::isInstance)
            .map(Engine.class::cast)
            .flatMap(engine -> Arrays.stream(engine.findChildren()))
            .filter(Host.class::isInstance)
            .map(Host.class::cast)
            .flatMap(host -> mapContainers(
                Arrays.stream(host.findChildren()), host.getName())
            )
            .collect(Collectors.toList());
}

private static Stream<ContextInfo> mapContainers(Stream<Container> containers,
    String hostname) {
    return containers
            .filter(Context.class::isInstance)
            .map(Context.class::cast)
            .map(context -> {
                ContextInfo info = new ContextInfo(context.getPath());
                info.setThisPart(context.getThisPart());
                info.setNotImportant(context.getNotImportant());
                info.setHostname(hostname); // The Bonus Challenge
                return info;
            });
}
Breaststroke answered 22/8, 2014 at 17:43 Comment(0)
K
1

First attempt beyond ugly. It will be years before I find this readable. Has to be a better way.

Note the findChildren methods return arrays which of course work with for (N n: array) syntax, but not with the new Iterable.forEach method. Had to wrap them with Arrays.asList

public List<ContextInfo> list() {
    final List<ContextInfo> list = new ArrayList<ContextInfo>();
    final StandardServer server = getServer();

    asList(server.findServices()).forEach(service -> {

        if (!(service.getContainer() instanceof Engine)) return;

        final Engine engine = (Engine) service.getContainer();

        instanceOf(Host.class, asList(engine.findChildren())).forEach(host -> {

            instanceOf(Context.class, asList(host.findChildren())).forEach(context -> {

                // copy to another object -- not the important part
                final ContextInfo info = new ContextInfo(context.getPath());
                info.setThisPart(context.getThisPart());
                info.setNotImportant(context.getNotImportant());
                list.add(info);
            });
        });
    });

    return list;
}

The utility methods

public static <T> Iterable<T> instanceOf(final Class<T> type, final Collection collection) {
    final Iterator iterator = collection.iterator();
    return () -> new SlambdaIterator<>(() -> {
        while (iterator.hasNext()) {
            final Object object = iterator.next();
            if (object != null && type.isAssignableFrom(object.getClass())) {
                return (T) object;
            }
        }
        throw new NoSuchElementException();
    });
}

And finally a Lambda-powerable implementation of Iterable

public static class SlambdaIterator<T> implements Iterator<T> {
    // Ya put your Lambdas in there
    public static interface Advancer<T> {
        T advance() throws NoSuchElementException;
    }
    private final Advancer<T> advancer;
    private T next;

    protected SlambdaIterator(final Advancer<T> advancer) {
        this.advancer = advancer;
    }

    @Override
    public boolean hasNext() {
        if (next != null) return true;

        try {
            next = advancer.advance();

            return next != null;
        } catch (final NoSuchElementException e) {
            return false;
        }
    }

    @Override
    public T next() {
        if (!hasNext()) throw new NoSuchElementException();

        final T v = next;
        next = null;
        return v;
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }
}

Lots of plumbing and no doubt 5x the byte code. Must be a better way.

Khanate answered 22/8, 2014 at 5:11 Comment(3)
My first ever lambda. Leaving here as a warning to everyone on what not to do.Khanate
This is an interesting illustration of the kind of code one might have to write if Java 8 had lambdas but not the Streams API.Conspecific
I'll take it :) I think it'll be a good reference on how Lambdas can be either used or abused. "This is why you need to learn the Streams API"Khanate

© 2022 - 2024 — McMap. All rights reserved.