What's the best way to optimize these nested for-loops used to create a hashmap in Java 8?

Question

Before I start, I've done tons of research on this and have yet to come to a solution. I have referenced the following threads, as well as many others, but I just wanted to demonstrate that I've searched long and hard for a solution before asking. My situation is more complicated than these:

How to convert nested for loop into Hashmap using java stream

How to iterate nested lists with lambda streams?

What is the proper way of replacing a nested for loop with streams in Java 8?

Okay, so I'm building a service that consumes another service that provides a class that I have no control over. I have to consume the class as it comes because it is dynamically generated from a file every time the project builds. It's a total mess with tons of nested classes and nested lists. The naming conventions were a mess too, but I've cleaned that up a lot here for readability. I need to dig really deep into the lists to get two values (code and cgyCode) from an object (Lower) and create a hashmap using one as the key (code) and one as the value (cgyCode).

I have FUNCTIONING logic that works as so:

//Code passing in object Rpn rpn which contains the nested lists        

HashMap<String, String> codeMap = new HashMap<String, String>();

            for (Object datDtl : rpn.getDatDtlOrRpnDtl()) {

                if (datDtl instanceof Rpn.DatDtl) {

                    for (Rpn.DatDtl.Upper upper : ((Rpn.DatDtl) datDtl).Upper()) {

                        for (Rpn.DatDtl.Upper.Lower lower : Upper.getLower()) {

                            codeMap.put(lower.getCode(), lower.getCgyCode());
                        }
                    }
                }
            }

So I've tried many variations to turn this entire thing into a lambda stream, if that is even the right thing to do. I haven't been able to get the entire thing working, there's always something that gets messed up. So the best I have is to stream the last loop, but nothing above. I feel that it can be consolidated way more than this.

FUNCTIONING:

HashMap<String, String> codeMap = new HashMap<String, String>();
                for (Object datDtl : rpn.getDatDtlOrRpnDtl()) {

                    if (datDtl instanceof Rpn.DatDtl) {

                        for (Rpn.DatDtl.Upper upper : ((Rpn.DatDtl) datDtl).Upper()){

                             upper.getLower().stream().forEach(lower -> codeMap.put(lower.getCode(), lower.getCgyCode()));

              }
         }
    }

I've tried many combinations of filtering, flatMaps, and streams, but I haven't had success converting all the functionality into into a series of lambda statements, nor do I know if that is the best course of action.

So my questions here are: what is the best way to optimize this code in terms of performance, and if not all of it can be optimized using lambdas and streams, from a learning perspective, why not?

Below I will demonstrate some of the other things I've tried that were close, but ultimately I couldn't get the mapping to resolve.

FAILED 1:

HashMap<String, String> codeMap = new HashMap<>();
rpn.getDatDtlOrRpnDtl().stream()
                    .filter(datDtl -> datDtl instanceof Rpn.DatDtl)
                    .flatMap(datDtl -> ((Rpn.DatDtl)datDtl).getUpper().stream()
                    .flatMap(upper -> upper.getLower().stream()
                    .forEach(lower -> codeMap.put(lower.getCode(), lower.getCgyCode()))));

The code above seems to work except for the .forEach function gives the error "no instance(s) of type variable(s) R exist so that void conforms to Stream". This is the same logic that is working in my functioning loop that incorporates the lambda, so that makes me feel the way I am arriving at "lower" is incorrect.

FAILED 2:

HashMap<String, String> codeMap = rpn.getDatDtlOrRpnDtl().stream()
                    .filter(datDtl -> datDtl instanceof Rpn.DatDtl)
                    .flatMap(datDtl -> ((Rpn.DatDtl)datDtl).getUpper().stream()
                    .flatMap(upper -> upper.getLower().stream()
                    .collect(Collectors.toMap(lower -> lower.getCode(), lower -> lower.getCgyCode())));

This code is similar to the one above. The error here is that ".getCode()" and ".getCgyCode()" aren't recognized as valid methods even though they are defined in the Lower class. This further suggests that I am correct in my theory that the way I arrive to "lower" is not the correct way to do so.

I have tried many other variations with .filter, .map, ect. But I am at a total loss here. It may not be possible to accomplish what I am trying to do, but if so, I would like to know why and what the next best thing would be.

Thanks to anyone who has read this far and wants to try to tackle this with me.

EDIT:

The solution led me to the answer, but in order to document for anyone else that may come across this, I will format the solution that ended up working:

Map<String, String> codeMap = rpn.getDatDtlOrRpnDtl().stream()
                    .filter(datDtl -> datDtl instanceof Rpn.DatDtl)
                    .flatMap(datDtl -> ((Rpn.DatDtl) datDtl).Upper().stream())
                    .flatMap(upper -> upper.getLower().stream())
                    .forEach(lower -> codeMap.put(lower.getCode(), lower.getCgyCode());

**FAILED 2** is the correct approach - except you forgot to assign it! `HashMap opTypCdMap = Rpn.getDatDtlOrRpnDtl().stream()` — Boris the Spider, Feb 02 '18 at 19:24
When you say I forgot to assign it, do you mean like so? HashMap codeMap = new HashMap<>(); Because that doesn't work, the methods still aren't recognized. Please note that the term in the map name was supposed to be codeMap. It is correct in the code, and I've corrected my post to reflect that. — Crislips, Feb 02 '18 at 19:30
this last part `.forEach(lower -> codeMap.put(lower.getCode(), lower.getCgyCode());` violates the side-effects... replace that with `Collectors.toMap(...)` — Eugene, Feb 04 '18 at 16:20

Mike Hill · Accepted Answer · 2018-02-02T20:01:40.167

4

Both of your attempts are very close.

Your code is not indented to show it, but you are placing some of your closing parentheses after the collect call in your failed attempts.

This isn't the correct order for the stream operations. The last operation should be a call to collect. As written, your code will produce a Stream as its result rather than a Map.

Try the following code instead:

Map<String, String> codeMap = Rpn.getDatDtlOrRpnDtl().stream()
                    .filter(datDtl -> datDtl instanceof Rpn.DatDtl)
                    .flatMap(datDtl -> ((Rpn.DatDtl) datDtl).Upper().stream())
                    .flatMap(upper -> upper.getLower().stream())
                    .collect(Collectors.toMap(lower -> lower.getCode(), lower -> lower.getCgyCode());

edited Feb 02 '18 at 20:01

answered Feb 02 '18 at 19:42

Mike Hill

3,622
23
27

This still has the same issue where the methods .getCode() and .getCgyCode() cannot be resolved for some reason. Oddly enough, if use your formatting, but replaced the .collect method with the .forEach method that I was able to use, it works! I would love to understand why this is. Also, would you be able to verify that this is the best course of action? I've read that lambdas and streams don't always improve performance, but it's difficult for me to tell when using them is appropriate. I'm under the impression that using a lambda in place of a loop pretty much always improves performance. – Crislips Feb 02 '18 at 20:18
To do it this way (with .forEach), I believe it is necessary declare the hashmap first though – Crislips Feb 02 '18 at 20:34
2

@Crislips, with regards to performance, streams generally do not increase performance unless they are used to distribute load across multiple threads (via the `.stream().parallel()` operation). If you look into the source code for streams (and you can -- it's all written in Java) you can see that there's nothing in there that will inherently improve performance. It's possible that JVMs will add optimizations at some point, but for now the primary benefit you get is readability and maintainability. It takes some time, but eventually streams can become very easy to read. – Mike Hill Feb 02 '18 at 21:38
@MikeHill Lambdas can be used to improve performance though, can't they? Is my solution actually improving performance or just changing readability? I was under the impression that it makes looping less costly to write things this way. Maybe streams don't improve performance, but they are typically utilized in lambdas that involve lists, which have the potential to improve performance, right? – Crislips Feb 02 '18 at 22:17
2

@Crislips lambdas or stream will not bring any performance benefit, it might be the other way around, usually they are used for readability. Performance might come into play when you have *lots* of elements and can parallelize the stream operation easily, but even then 0 you would have to measure to prove – Eugene Feb 04 '18 at 16:21
@Eugene There must be more to it than just readability. I'm not referring to just this situation specifically. I feel like if it's really just readability, then it's kind of arbitrary because it's just new syntax to learn. It doesn't seem particularly easier to read this, it's roughly the same amount of characters. A senior dev on my team suggested using lambdas and streams for performance benefit. I'm not disagreeing with you, I'm just confused as I feel I'm getting mixed information about these functions. Other online sources suggest performance improvement as well. – Crislips Feb 05 '18 at 19:12
@Crislips invite you senior dev to this discussion, it's only a click away! and `words` from a senior dev are zero, compared to `facts` from senior devs... 1) Once you will get comfortable with this syntax, you will not be able to get back. 2) *For this example* the amount of characters is close to be the same, there are examples where the diff is huge. 3) just recently this has been discussed with actual numbers (that your senior dev can probably explain to you ;) ) https://stackoverflow.com/questions/48519346/why-my-string-manipulation-is-slow-using-lambda-expression/48624167#48624167 – Eugene Feb 05 '18 at 19:36
@Eugene I will definitely have a discussion with him about this. Thanks for the tips are resources, this is quite the learning experience for me! – Crislips Feb 05 '18 at 20:14
See [this question](https://stackoverflow.com/questions/22658322/java-8-performance-of-streams-vs-collections) for benchmark information on Java 8 streams. Brian Goetz's (a Java Language Architect) [highly-rated comment](https://stackoverflow.com/questions/22658322/java-8-performance-of-streams-vs-collections#comment36791431_22670380) on the top answer is especially relevant regarding JVM optimizations. – Mike Hill Feb 07 '18 at 22:18
The important thing is to avoid pre-optimization and to lean toward readability and maintainability first. Do not worry about micro-optimzation until necessary, and then use profiling to determine where these optimizations will have the most important effect. Micro-optimizations often have the cost of reducing maintainability, which can lead to increased development time and increased potential for bugs. In addition, micro-optimizations are often less effective than expected (and sometimes even have a negative effect instead). Effects of micro-optimizations should be measured and verified. – Mike Hill Feb 07 '18 at 22:25

What's the best way to optimize these nested for-loops used to create a hashmap in Java 8?

1 Answers1