-1

I need some alternative for the following code to be more readable. ssrFlightList is nested list.

I tried with forEach:

ssrFlightList.stream().forEach(
            x -> x.getSsrPassengerList().stream().forEach(
                y -> y.getSsrItemList().stream().forEach(
                    z -> z.getSsrCodeList().stream().forEach(t -> {
                        if ("BSML".equalsIgnoreCase(t.getCode())) {
                            ssrCodeListStr.set(t.getText());
                        }
                    })
                )));

And this is flatMap example too:

Optional<SSRItemCode> itemCode = ssrFlightList
        .stream()
        .flatMap(firstNode -> firstNode.getSsrPassengerList().stream())
        .flatMap(s -> s.getSsrItemList().stream())
        .flatMap(s -> s.getSsrCodeList().stream())
        .filter(s1 -> s1.getCode().equals("BSML"))
        .findFirst();

itemCode.isPresent() ? ssrCodeListStr.set(itemCode.get().getText());

I'm looking for an alternative way to get ssrCodeListStr text.

Vinay Prajapati
  • 7,199
  • 9
  • 45
  • 86
Arif Acar
  • 1,461
  • 2
  • 19
  • 33
  • 5
    "I'm looking for an alternative way" Why? What is deficient about these two ways? – Andy Turner Mar 12 '18 at 08:27
  • 3
    Note that your second example isn't the same as the first, since the first takes that *last* matching value, whereas the second gets the first matching value. – Andy Turner Mar 12 '18 at 08:28
  • I'm not sure you can do much better. You could shorten the second one slightly by writing a helper method that turns a method reference into a stream. – Oliver Charlesworth Mar 12 '18 at 08:28
  • @AndyTurner you right but ssrCodeList contains just one "BSML" string. – Arif Acar Mar 12 '18 at 08:30
  • you should devide the content of each map in small functions , every function ist the logic inside the maps. You should give function a clear name to know what it does . It would be clearer und easy to debug. – Ayoub Abid Mar 12 '18 at 08:32
  • @ArifAcar it is computationally inefficient even if there is only one BSML value as you will keep iterating through this multidimensional data structure after you already found the value. As for your question: there is no much better solution -- you need to iterate over all dimensions of your data whether you use loops or streams – Oleg Sklyar Mar 12 '18 at 08:33
  • @OlegSklyar it's definitely computationally inefficient in the first case. The `flatMap` approach can (at least in theory) stop as soon as it finds something. – Andy Turner Mar 12 '18 at 08:35
  • 4
    Nit: `....findFirst().map(t -> ssrCodeListStr.set(t.getText()))`. – Andy Turner Mar 12 '18 at 08:36
  • If you really need to search the most deeply nested list, consider building an index (i.e. a `Map`) of those nodes, especially as you say the nodes are unique. – Jim Garrison Mar 12 '18 at 08:38
  • 1
    @AndyTurner `....findFirst().ifPresent(t -> ssrCodeListStr.set(t.getText()))` – fps Mar 12 '18 at 15:16

2 Answers2

0

There is no Java 8 magic wand to make things more readable. Just use method references and give the methods meaningful names:

ssrFlightList.stream().forEach(this::setSsrForAllPassengers);

private void setSsrForAllPassengers(Flight flight)
{
    flight.getSsrPassengerList().stream().forEach(this::setSsrForPassenger);
}

private void setSsrForPassenger(Passenger passenger)
{
    passenger.getSsrItemList().stream().forEach(this::setSsrForItem);
}

private void setSsrForItem(Item item)
{
    //etc....
}
Michael
  • 41,989
  • 11
  • 82
  • 128
  • 1
    It is very questionable if this is more readable. Given that these methods introduce multiple different levels of abstraction it is quite against the clean code concepts – Oleg Sklyar Mar 12 '18 at 09:56
  • *"It is very questionable if this is more readable"* Disagree. *"Given that these methods introduce multiple different levels of abstraction"* what? *"it is quite against the clean code concepts"* which? – Michael Mar 12 '18 at 10:07
  • 1
    Sorry, I meant different level of abstraction rather than indirection. With this correction: p. 36 -- One Level of Abstraction per Function (your functions are interdependent and nested), p. 44 -- Have No Side Effects (your functions set something or some object, both are totally not obvious from their definitions). Even without those references the code is more difficult to read than the fluent API of individual flatMaps. – Oleg Sklyar Mar 12 '18 at 10:54
  • @OlegSklyar **Re. pg 36**: Each function here only contains a single statement so it's impossible for them to contain different levels of abstraction. **Re. pg 44:** What OP is asking for almost necessarily involves side-effects. Thank you for your input but I do not wish to discuss this further because you have no idea what you're talking about. – Michael Mar 12 '18 at 11:02
0

An alternative way to the first is to use plain old enhanced for loops: streams are adding no value in this case. They allow you to omit the type (*), but add lots of other syntactic choss instead.

for (Flight x : ssrFlightList) {
  for (Passenger y : x.getSsrPassengerList()) {
    for (Ssr z : y.getSsrItemList()) {
      for (SsrCode t : z.getSsrCodeList()) {
        if ("BSML".equalsIgnoreCase(t.getCode()) {
          ssrCodeListStr.set(t.getText());
        }
      }
    }
  }
}

Readability is subjective, but I just find that a lot less cluttered than trying to use streams; and it's more flexible, because you aren't as constrained in what you can do in the loops (e.g. throwing checked exceptions, breaking, returning etc).

If you did want to use something functional here, you might consider pulling these loops into a method, and injecting a Consumer<String> (or Consumer<SsrCode> or whatever) to allow you to reuse the code to do other things than set this particular field:

findSsrCode("BSML", ssrCodeList::set);
findSsrCode("XXXX", otherField::set);

I think that the flatMap version is fine, and would probably use that myself; but there is nothing egregiously wrong with the for loops approach.

(*) But since Java 10 is now released (or is on the cusp of release), you can use var here instead.

Andy Turner
  • 137,514
  • 11
  • 162
  • 243