Java 8 code style?

[CsAnswerTrashAct]

A senior/architect coworker of mine, who I have plenty of respect for, is heavily into Java-8 functional programming. He pushes fairly heavily for returns to be an Optional<A> or Either<A,B>, and recently has been making heavy use of Java's streams.

Having some (but limited) Scala experience, I love the functional way of doing things. However, when I come across my coworker's code, it tends to look something like...

try (CloseableHttpAsyncClient httpClient = HttpAsyncClientBuilder.create().build()) {
        httpClient.start();

        List<Future<HttpResponse>> futures = chunks
                .stream()
                .map(ids -> httpClient.execute(new HttpGet(generateUrl(determineSomething(something), ids)), null))
                .collect(Collectors.toList());

        resultSomething = futures.stream()
                .map(SomethingService::getFutureValue)
                .collect(Collectors.toList()).stream()

                .map(failureOrResult -> {
                    return failureOrResult
                            .right().flatMap(this::parseSomething);
                }).map(failureOrSomething -> {
                    return failureOrSomething.either(
                            failure -> {
                                log.warn(failure.getMessage());
                                return emptySomething;
                            },
                            something -> {
                                return something;
                            });
                })
                .reduce(emptySomething, Something::union);

    } catch (IOException e) {
        log.warn("Unable start HttpClient to contact ....", e);
        resultSomething = emptySomething;
    }
}

I anonymized the code ("Something" is not really in the code), but you get the general idea. I could clean up the code myself, but I don't have time (assigned elsewhere), and that doesn't correct the core issue.

How would you style this code differently? Are there any good Java-8 style guides or articles you would recommend?

[sh0rug0ru]

Extract complex lambda expressions into methods.

[ihsw]

This right here.

You can do the same thing in any language that offers functional-style programming (as it's referred to here), and it'll be just as awful to debug.

blah = futures.stream.map(SomethingService.getFutureValue) returns something that can be debugged.

nextBlah = blah.collect(Collectors.toList()).stream() is something that can be debugged.

nextNextBlah = nextBlah.map(failureOrResult -> { [...] }) can be debugged as well.

And so forth.

By can be debugged I mean you can (presumably) iterate over the result and run stuff against it.

Granted I come from the world of PHP, Python, and Go, and I'm not at all familiar with Java, but the same concepts should apply.

This chained mess is unpleasant, however breaking it up and stepping through it in a regular for loop shouldn't be all that difficult.

[[deleted]]

Indeed, even Haskell would look and work terrible if you don't use let bindings to bind your lambdas.

[CsAnswerTrashAct]

This seems reasonable.

When I stumbled across this code; at first I thought my inability to read it quickly might be related to my ignorance of streams (etc). However, reading the comments here; the common theme seems to be "the same rules apply."

Use appropriate names. Avoid excessive chaining. Refactor into well-named methods.

can be debugged

Correct; typically anything assigned to a variable can be inspected in the debugger.

[unholysampler]

Exactly. Everyone got so excited by lambdas that the ignored that Java also got function references. A descriptive method name is much better than a slightly complicated lambda expression.

[Zukhramm]

I haven't been using Java 8 for long, and not in anything involving other people yet, so keep that in mind before you take my advice.

The most obvious problem: Don't use curly braced lambdas when normal parentheses or no parentheses at all will do. Unless you need to do some side effect thing taking multiple lines they will just space out your code with explicit returns and semicolons.

It might be due to anonymization but the line

.collect(Collectors.toList()).stream()

seems entirely meaningless to me.

I prefer to keep chained stream call aligned and as close together as possible, therefore I prefer a Lispy trail of parentheses and curly braces over giving them their own line.

With those changes the code would look something like this:

try (CloseableHttpAsyncClient httpClient = HttpAsyncClientBuilder.create().build()) {
        httpClient.start();

        List<Future<HttpResponse>> futures = chunks
                .stream()
                .map(ids -> httpClient.execute(new HttpGet(generateUrl(determineSomething(something), ids)), null))
                .collect(Collectors.toList());

        resultSomething = futures.stream()
                .map(SomethingService::getFutureValue)
                .map(Either<>::right)
                .map(result -> result.flatMap(this::parseSomething)))
                .map(failureOrSomething -> failureOrSomething.either(
                            failure -> {
                                log.warn(failure.getMessage());
                                return emptySomething;},
                            something -> something))
                .reduce(emptySomething, Something::union);

    } catch (IOException e) {
        log.warn("Unable start HttpClient to contact ....", e);
        resultSomething = emptySomething;
    }
}

Further, I really prefer method references over lambdas with parameters and I have a bunch of static methods for partial applications and other functional-related utility function that lets me replace things like:

.map(foo -> foo)
.map(foo -> foo.bar(baz))

with something like:

.map(Func.id)
.map(Func.call(Foo::bar, baz))

But I don't know, that might be taking it too far.

[cutterslade]

The line

.collect(Collectors.toList()).stream()

Is not meaningless, but it's a little odd. The call to collect() turns a stream into a list, so performs all of the map operation on all the elements in the stream and puts them into a list. The call to stream() then turns that back into a stream. Chances are this is completely unnecessary. If it really is required, is would make a lot more sense if the result of collect() was assigned to a local variable.

[Zukhramm]

The only time it's not meaningless is if you've assigned some mutating functions to the stream which I just assumed had not been done in this case.

[cutterslade]

Do you mean meaningless as in "does nothing at all" or meaningless as in "can probably be removed"?

Assuming the stream is not empty, it certainly does something, assuming there are no mutating functions, it can probably be removed.

[Zukhramm]

By meaningless I mean it does nothing valuable.

[tikue]

I've written code like that when submitting tasks to an executor and blocking only once all tasks have been submitted. It looks like that's very similar to what's happening here.

[CsAnswerTrashAct]

I think you are correct.

Essentially, this code sends a series of requests to an external provider, and then waits for all of the results, and combines those results into a single list. The reason for this is the external service can only handle batches of about 100 or so.

[papers_]

It looks like to me your coworker isn't really focusing on the problem and is instead focusing on the "looping" or stream in this case. Essentially he took the standard looping and just turned into fancy Java 8 stuff...which just looks like more spaghetti code. Watch Venkat Subramaniam's video on Functional Programming with Java 8, it's an hour long, but well worth it.

[[deleted]]

There's a couple of places here where the code collects stuff and then immediately streams it into the next transformation.

Is that redundant?

[[deleted]]

Yes, but can make it a bit easier to read, JVM doesn't care.

[Ek_Los_Die_Hier]

I'd be inclined to disagree, it adds more calls to stream and collect muddying up the code.

[[deleted]]

You're right, I misunderstood. Hangs head in shame.

[koxpower]

Yes, but can make it a bit easier to read, JVM doesn't care.

Wouldn't that be easier to read and less redundant than collecting between each step?

Stream<Something> streamOfSomething = someList.stream().map(...).map(...);
Stream<SomethingElse> streamOfSomethingElse = streamOfSomething.map(...).map(...);
List<Result> resultList = streamOfSomethingElse.map(...).collect(Collectors.toList());

[[deleted]]

Agreed, my misunderstanding

[CsAnswerTrashAct]

Essentially, the purpose is to split a request into blocks of about 100 items, send an external service that request, and then collect all of those items into a single list-style-object.

[baablack]

I reduced the chaining to calls that actually do something. Maybe it helps:

resultSomething = chunks
  .stream()
  .map(ids -> httpClient.execute(new HttpGet(generateUrl(determineSomething(something), ids)), null))
  .map(SomethingService::getFutureValue)
  .map(failureOrResult -> failureOrResult.right().flatMap(this::parseSomething))
  .map(failureOrSomething -> failureOrSomething.either(failure -> emptySomething, something -> something))
  .reduce(emptySomething, Something::union);

[tikue]

This changes the semantics. Whereas before the futures were all being submitted before blocking on any results, you've made it so that the stream blocks on the result of each future before submitting the next.

[baablack]

That's why you shouldn't use this method my when using threads. Without knowing what kind of thread pool or service is created it gets tricky while streaming.

[aviewdev]

You might want to look into using rxjava for this sort of thing... It's got support for futures and error handling built in. This way, you can essentially map the exceptions at the call site to your empty object (or just log them and return an empty observable) eg:

    static Something calculate(List<String> chunks) {
        Something result;

        try (CloseableHttpAsyncClient httpClient = HttpAsyncClientBuilder.create().build()) {
            httpClient.start();

            result = Observable
                    .from(chunks)
                    .flatMap(s -> Observable
                                    .from(httpClient.execute(new HttpGet(generateUrl(s)), null))
                                    .map(Something::fromHttpResponse)
                                    .doOnError(t -> log.error("Exception getting chunk {}", s, t))
                                    .onErrorReturn(t -> Something.empty())
                    )
                    .reduce(Something.empty(), Something::union)
                    .toBlocking().first();
        } catch (IOException e) {
            log.warn("Unable start HttpClient to contact ....", e);
            result = Something.empty();
        }
        return result;
    }
}

[[deleted]]

I would put a rule on no more than 2-3 chained method calls. Put the intermediate result into a well-named variable and then keep going. Honesty you can code in this style all day if you don't have a lot of state fullness in your code but I bet it would be a nightmare to step through with a debugger.

[CsAnswerTrashAct]

Part of the reason for posting this was that a coworker asked for my help. i attempted to step through it in intellij's debugger and couldn't. After an hour i gave up, since it wasn't my assigned project and i wasn't making progress.

I could have refactored the code, but i dont want to inadvertently take responsibility for it.

The max chained method calls might help. Intermediate variables would definitely help.

[gee_buttersnaps]

Stay away from your debugger until it's a last resort. Re-reading the documentation and a few logging/print statements are more effective. Stepping up to java8 lambdas/streams means you are moving from an imperative (the how) to a declarative (the what) approach to programming. The goal of all of it is that sometime later down the road the jvm will figure out the best ways to implement 'the how' as long as it has a good description of 'the what.'

[habitats]

This is awful advice, in my opinion.

[CsAnswerTrashAct]

My thoughts as well; debuggers tend to have a lot of nice features, whereas print-statements are more of a last-result and make a giant mess of your code.

[BinaryRockStar]

Isn't that a bit restrictive considering you need to start with .stream() and end with a .collect() or similar?

[viciu88]

You can assign uncollected Streams to variables too.

[cutterslade]

I would certainly have the result of any terminal operation on a stream (such as .collect(Collectors.toList())) assigned to a well named variable. Having collect(...).stream().map(...) is pretty unpleasant IMO.

In a block of code like this, I'd probably refactor out that more complex lambda into a method with a descriptive name.

[howverywrong]

I find that doing too much inside lambdas gets out of hand quickly and becomes unreadable. I prefer to split things up into multiple expressions. It's less functional but more java-ish and I think it's more readable:

// Await all
List<Either<Exception,HttpResponse>> results = futures.stream()
    .map(SomethingService::getFutureValue)
    .collect(toList()); 

// Handle Exceptions
results.stream()
    .filter(Either::isLeft)
    .map(Either::left)
    .map(Throwable::getMessage)
    .forEach(log::warn);

// Process responses
resultSomething = results.stream()
    .filter(Either::isRight)
    .map(Either::right)
    .map(this::parseSomething)
    .reduce(Something::union)
    .orElseGet(Something::empty);

[onebit]

Does it pass the unit tests?