8

I would like to optimize the below code to take an existing ArrayList and make a new list based on the below logic. Does streaming/lambda in Java 8 make it possible?

for (ShipmentTracingDTO tracing : tracings) {
    if (tracing.getDestination()) {
        newTracings.add(tracing);
        break;
    }
    newTracings.add(tracing);
}
Lii
  • 11,553
  • 8
  • 64
  • 88
AhmedBakran
  • 135
  • 1
  • 8
  • Possible duplicate of https://stackoverflow.com/questions/20746429/limit-a-stream-by-a-predicate – Jan Gassen Jan 10 '19 at 07:58
  • 1
    @JanGassen well, I initially suggested using `takeWhile`, but then the part where the element even when the condition is met is `add`ed is trickier since the `takeWhile` would ignore that specific element. – Naman Jan 10 '19 at 08:04
  • This looks like an [XY Problem](https://meta.stackexchange.com/questions/66377/what-is-the-xy-problem). It is not a typical scenario to want to create a list of all elements not meeting a certain condition before one that meets the condition, and then add the latter too. What exactly are you trying to do this for? – Klitos Kyriacou Jan 10 '19 at 09:53
  • 3
    Your loop would be much simpler when you did `for (ShipmentTracingDTO tracing : tracings) { newTracings.add(tracing); if(tracing.getDestination()) break; }` – Holger Jan 10 '19 at 13:30
  • 2
    When you say optimize, do you mean for speed, or code verbosity? Streams can be slower than loops. So you are often better off with loops if you are looking for speed. But using streams and lambdas does look cooler. https://medium.com/@milan.mimica/slow-like-a-stream-fast-like-a-loop-524f70391182 –  Jan 10 '19 at 13:52

3 Answers3

9

You can do it using IntStream.range and List.subList possibly as :

List<ShipmentTracingDTO> newTracings = tracings.subList(0, IntStream.range(0, tracings.size())
       .filter(i -> tracings.get(i).getDestination())
       .map(a -> a + 1) // to include the index of occurrence
       .findFirst()
       .orElse(tracings.size())); // else include all until last element

Note: This is just a view of tracings and hence changing the underlying list tracings would also reflect in changes in newTracings. If you want to decouple these, you can create a new ArrayList<>() from the solutions.

Edit: In comments from Holger, you could alternatively use:

List<ShipmentTracingDTO> newTracings = IntStream.rangeClosed(1, tracings.size())
        .filter(i -> tracings.get(i - 1).getDestination()) // i-1 to include this as well
        .mapToObj(i -> tracings.subList(0, i)) // Stream<List<ShipmentTracingDTO>>
        .findFirst() // Optional<List<ShipmentTracingDTO>>
        .orElse(tracings); // or else the entire list
Naman
  • 27,789
  • 26
  • 218
  • 353
  • 2
    Or `List newTracings = IntStream.rangeClosed(1, tracings.size()) .filter(i -> tracings.get(i - 1).getDestination()).mapToObj(i -> tracings.subList(0,i)) .findFirst() .orElse(tracings);` If a new list is required, just wrap the entire expression in a `new ArrayList<>( … )` – Holger Jan 10 '19 at 13:48
  • 1
    @Holger Thanks, the `i-1` part was something that was just not striking my mind while I was thinking to solve this one. Maybe, the same thing to remind me to write the iterative first, optimize it to the best and then approach streams. Thanks again anyway. – Naman Jan 10 '19 at 13:59
  • Both these solutions are much more complicated and harder to read than the original for-loop. Streams clearly is no suitable tool for solving this problem! – Lii Jan 10 '19 at 15:19
  • 1
    @Lii Well, I could say its just a perception. Since the use-case that OP is using such a code is not justified either. Yes agreed, if it has to be this way, the non-stream code looks way better. – Naman Jan 10 '19 at 16:30
4

If I needed such processing I would divide it into two separate streams:

final int lastElemPosition = tracings.size() - 1;
final int firstElemWithGetDestinationTruePosition = IntStream.range(0, lastElemPosition)
    .filter(i -> tracings.get(i).getDestination())
    .findFirst()
    .orElse(lastElemPosition);
final List<ShipmentTracingDTO> desiredElements = IntStream.rangeClosed(0, firstElemWithGetDestinationTruePosition)
    .mapToObj(tracings::get)
    .collect(Collectors.toList());
4

First, I would point out there's not much wrong with the original code that you want to change. Just because Java has streams, you don't have to use them.

To use streams, I would break up the solution into two phases:

OptionalInt indexOfMatchingItem = IntStream.range(0, tracings.size())
        .filter(i -> tracings.get(i).getDestination())
        .findFirst();

List<ShipmentTracingDTO> newTracings = new ArrayList<>(
        indexOfMatchingItem
                .map(i -> tracings.subList(0, i + 1))
                .orElse(tracings));

The above can be written as a single expression, but splitting it with a suitably named intermediate variable can make the code self-documenting.

I've created a new ArrayList so that it won't be affected by any subsequent changes to the original list tracings. If tracings is immutable, you can skip the construction of a new ArrayList.

The above solution is slightly better performing than the original code in the question, because the ArrayList constructor pre-allocates an array of exactly the required size, and so avoids the overhead of resizing and multiple array copies.

An alternative (perhaps more readable but less performant) solution is:

List<ShipmentTracingDTO> newTracings =
        Stream.concat(
            tracings.stream().takeWhile(i -> !tracings.get(i).getDestination()),
            tracings.stream().dropWhile(i -> !tracings.get(i).getDestination()).limit(1)
        ).collect(toList());
Klitos Kyriacou
  • 10,634
  • 2
  • 38
  • 70
  • 1
    Or `newTracings = new ArrayList<>(indexOfMatchingItem.isPresent()? tracings.subList(0, 1 + indexOfMatchingItem.get()): tracings)`. The performance does not only benefit from pre-sizing the array, when the source is an `ArrayList`, there will be a single `Arrays.copyOf` operation from the source list’s array. – Holger Jan 10 '19 at 14:13
  • Thanks, Holger. I've added your suggestions. Note that `list.subList(0,list.size()) == list` (by identity) in most implementations of `subList`, so the two variations are equivalent. The `isPresent` call is often considered a "code smell" and the whole smell of the solution seems to suggest it's a weird requirement - it's not as obvious in the original for-loop version but it becomes more obvious when you try to turn it into functional code. – Klitos Kyriacou Jan 10 '19 at 14:44
  • `list.subList(0,list.size()) == list` does not hold for the commonly used `ArrayList`, for example. I’m not even sure whether the contract would allow this optimization as subsequently adding an element to the original list invalidates the sublist rather than show through. To those considering `isPresent` as code smell, `new ArrayList<>(indexOfMatchingItem.map(i -> tracings.subList(0, 1 + i)).orElse(tracings))` would do, but lose the advantage of not creating a new object. – Holger Jan 10 '19 at 14:54