In the following code I am trying to remove all nodes and leaves that do not have a root in the key of input map. Input is the Map<rootId: String, listOf(root,nodes,leaves)>

Working logic

@NotNull
private static Map<String, List<Element>> removeOrphanNodes(Map<String, List<Element>> mapOfAllProcesses) {
    Map<String,List<Element>> refinedRootMap= new HashMap<>();

    for(Map.Entry<String,List<Element>>entrySet: mapOfAllProcesses.entrySet())
    {
        if(entrySet.getValue().size()>1)
            refinedRootMap.put(entrySet.getKey(),entrySet.getValue());
        else {
            Element loneElement = entrySet.getValue().get(0);
            if (entrySet.getKey().equals(loneElement.getIdAsString()))
                refinedRootMap.put(entrySet.getKey(),entrySet.getValue());
            else if(loneElement.getCurrentOperations()!=null && loneElement.getCurrentOperations().iterator().next().getId().toHexString().equals(entrySet.getKey()))
                refinedRootMap.put(entrySet.getKey(),entrySet.getValue());
        }
    }
    return refinedRootMap;
}

The above code works as expected. I wanted to make use streams to achieve the same functionality but getCurrentOperations throws null pointer

My Attempt

return mapOfAllProcesses.entrySet().stream().filter(entry -> entry.getValue().size()>1 || entry.getValue().stream()
            .anyMatch(
                    element-> element.getIdAsString().equals(entry.getKey())||element.getCurrentOperations().stream().findFirst().get().getId().toHexString().equals(entry.getKey())
            )).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));

CodePudding user response：

Don't do this.

The Stream API is not a replacement for loops or conventional iteration constructs.

Ideal code for putting into a Stream would be something that:

• Does one function (reads data out, loads data in)
• Could be parallelized
• Has no side-effects (e.g. doesn't reach out to anything else)

Your code satisfies the last bullet, I'm not sure about the middle bullet and it definitely does a lot more based on conditions, which...isn't ideal for streaming.

Maybe a better way to approach this problem would be to re-think the data structure you're using? You're using a Map<K, List<V>>, and that can be contextualized inside of a Guava Multimap. Maybe that's where the first improvement needs to happen - using a more suitable data structure for this instead?

CodePudding user response：

To avoid raising NPE when collection returned by element.getCurrentOperations() is null you might try to use Stream.ofNullable(). And dummy default value while extracting result from the optional can help in cases when this collection is empty (and consequently optional would be empty).

return mapOfAllProcesses.entrySet().stream()
    .filter(entry -> entrySet.getValue().size() > 1 ||
        entry.getValue().stream()
        .anyMatch(
            element -> element.getIdAsString().equals(entry.getKey())
                ||
            Stream.ofNullable(element.getCurrentOperations()).findFirst()
                .map(operation -> operation.getId().toHexString())
                .orElse("").equals(entry.getKey())
    ))
    .collect(Collectors.toMap(
        Map.Entry::getKey,
        Map.Entry::getValue
    ));

It is worth to note that both snippets (imperative and functional) are extremely convoluted, so it would be wise to consider extracting some pieces of functionality into separate methods.

CodePudding user response：

This should do it, but I don't think there's any advantage to be gained this way. It's slightly shorter, but not really any simpler, and definitely not any clearer.

@NotNull
private static Map<String, List<Element>> removeOrphanNodes(Map<String, List<Element>> mapOfAllProcesses) {
    return mapOfAllProcesses.entrySet().stream().filter((entry) -> 
            (entry.getValue().size() > 1)
                || entry.getKey().equals(entry.getValue().get(0).getIdAsString())
                || Optional.ofNullable(entry.getValue().get(0).getCurrentOperations())
                           .stream()
                           .map((ops) -> ops.iterator().next().getId().toHexString())
                           .anyMatch((s) -> s.equals(entry.getKey()))
        ).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}

This version handles the fact that getCurrentOperations() can return null by capturing the return value as an Optional. The Optional's stream will be empty when getCurrentOperations() returns null, and anyMatch() returns false on an empty stream.

The map() of the inner stream could be omitted in favor of a more complex predicate for its anyMatch(), but I think the version with map() is clearer.