Clean Streaming Code

Close up of water gushing out of the pipe

Streaming is an awesome tool. It helps to separate concerns (e.g. iteration, filtering, execution), to avoid state and thus improves thread safety. Furthermore we can express same functionality  with less code. But it’s hard to read and to test.

Really?

In a recent project we introduced Java 8 streaming to legacy code and converted a lot of loops. Although I’m quite used to write code that makes use of Java 8 streaming API, I realized how actually hard it was to read such a code written by others (and thus how hard my code might be to read by others). Furthermore there were bugs in the code although the tests were all green and were covering 100% of the lines. These tests were quite complicated as they had to test the whole method including iteration, filtering and execution, so a lot of concerns.

The following shows the legacy code.

private void addLinksToTarget(List<Combination> links,
        Combination combination, String caseDesc) {
    for (Iterator<Combination> linksIterator = links.iterator(); links.hasNext()
            &amp;&amp; combination.getTargets().size() < PLACES_PER_KEYWORD;) {
        Combination link = linkIterator.next();
        if (!link.getSegment().equals(combination.getSegment())) {
            continue;
        }

        Rating linkLimit = link.getRatingByType(RatingType.AVAILABLE_PLACES);

        if (linkLimit.getValue() <= 0) {
            linkIterator.remove();
            continue;         
        }
        // avoid duplicate urls in the same footer
        String sourceUrl = urlFormatter.format(combination);
        String targetUrl = urlFormatter.format(link);
        if (targetUrl.equals(sourceUrl)) {
            continue;
        }
        boolean alreadyContained = false;
        for (String footerPart : combination.getTargets()) {
            if (StringUtils.containsIgnoreCase(footerPart, targetUrl)) {
                alreadyContained = true;
                break;
            }
        }
        if (alreadyContained) {
            continue;
        }
        String target = link.getLabel()
                        + ">" + targetUrl
                        + ">" + link.getPriority()
                        + ">" + caseDesc;
        combination.getTargets().add(target);
        linkLimit.setValue(linkLimit.getValue() - 1);
    }
}

A fellow dev of mine took the challenge and transformed the original version to the following using Java 8 streaming.


private void addLinksToTarget(List<Combination> links,
        Combination combination, String caseDesc) {
    links.stream()
         .filter(link -> link.getSegment().equals(combination.getSegment()))
         .filter(link -> link.getRatingByType(AVAILABLE_PLACES)
                         .orElse(new Rating(AVAILABLE_PLACES, 0)).getValue() > 0)
         .filter(link -> !isSelfReference(combination, link))
         .filter(link -> !isAlreadyContainedInTargets(link, combination))
         .limit(linksPerCombination - combination.getTargets().size())
         .forEach(link -> {
                String targetUrl = urlFormatter.format(placeable);
                String target = link.getLabel()
                                + ">" + targetUrl
                                + ">" + link.getPriority()
                                + ">" + caseDesc;
                combination.getTargets().add(target);
                Rating linkLimit = link.getRatingByType(AVAILABLE_PLACES).get();
                linkLimit.setValue(linkLimit.getValue() - 1);
            });
    links.removeIf(link -> {
        Optional<Rating> rating = link.getRatingByType(AVAILABLE_PLACES);
        return !rating.isPresent() || rating.get().getValue() <= 0;
    });
}

This code now looks much better, but it’s still hard to read – at least was for me as an outsider. So I started refactoring the code to a more readable form. I created predicates containing the filter logic and give them meaningful names and extracted the business logic executed in forEach to a consumer.


 private void addLinksToTarget(List<Combination> links,
        Combination combination, String caseDesc) {
    Helper helper = Helper.of(combination, urlFormatter, caseDesc);
    links.stream()
         .filter(helper.sameSegments)
         .filter(helper.hasLinkPlacesLeft)
         .filter(helper.notSelfReference)
         .filter(helper.linkNotContained)
         .limit(helper.maxAvailableLinks)
         .forEach(helper.addLinkToTargetWithCaseDescription);
    links.removeIf(helper.noLinkPlacesLeft);
}

Every condition and the consumer are now easily (and isolated) testable and possible bugs could be found faster. But most important: I easily can understand, what this code block is intended to do, even if I wasn’t the author.

Conclusion

We learned great things from Uncle Bob like moving blocks of logic into self-describing methods or not being done before the code is refactored to be well readable by others (including my future self in half a year). Apparently we sometimes forget these learnings when it comes to new features like the streaming API of Java 8. Maybe it’s because we’re so excited to have them and think it’s all much clearer now – or simply because we need to gather some more experience.

This blog post has also been published on the eBay technology blog (DE)

Leave a Comment