14

I have a simple stream like following:

List<Long> r = l.stream()
                .filter(a->a.getB() % 2 == 0)
                .map(A::getB)
                .collect(Collectors.toList());

But Intellij suggests me:

'filter()' and 'map()' can be swapped Inspection info: Reports stream API call chains which can be simplified. It allows to avoid creating redundant temporary objects when traversing a collection. e.g.

  • collection.stream().forEach() → collection.forEach()
  • collection.stream().collect(toList/toSet/toCollection()) → new CollectionType<>(collection)

The example given by Intellij is easy to understand, but I don't understand why it suggests me to map().filter().

I view the source of ReferencePipeline but find no clue: map().filter() or filter().map() makes no difference when comes to the temporary object related to stream implementation (filter().map() will have less auto-boxing if A.b is a primitive which makes me more confused).

So, am I missing some point of stream implementation or this is a false alarm of Intellij?

Gautham M
  • 4,816
  • 3
  • 15
  • 37
Tony
  • 5,972
  • 2
  • 39
  • 58

1 Answers1

18

a.getB() is invoked twice - once inside the filter and it is also the mapping function, so instead of doing this twice, it would be better to first map it using getB and then filter it out

List<Long> r = l.stream().map(A::getB).filter(b->b % 2 == 0).collect(Collectors.toList());

EDIT

If getB returns a long then mapToLong could be used to avoid intermediate boxing operations.

List<Long> r = l.stream()
                .mapToLong(A::getB)
                .filter(b->b % 2 == 0)
                .boxed()
                .collect(Collectors.toList());

SAMPLE OUTPUT

Using a static counter to count the invocation of get method:

class A {
    public static int count = 0;
    private long b;

    public long getB() {
        count++;
        return b;
    }
}
List<A> list= List.of(new A(1L), new A(3L), new A(4L));
list.stream() 
    .filter(a -> a.getB()%2 == 0)
    .map(A::getB)
    .collect(Collectors.toList());
System.out.println(A.count); // returns 4

whereas

list.stream()
    .mapToLong(A::getB)
    .filter(b->b % 2 == 0)
    .boxed()
    .collect(Collectors.toList());
System.out.println(A.count); // returns 3
Gautham M
  • 4,816
  • 3
  • 15
  • 37
  • But what is the exact optimization here ? Let's say we have billions of elements in the list and %10 of the elements are even... Wouldn't be filtering before mapping all elements more optimized. (I mean CPU cycles) – Ali Can Apr 07 '21 at 05:05
  • @AliCan the catch is that you need to call the same getter in order to filter, which doesn't avoid the redundancy mentioned in the post. – ernest_k Apr 07 '21 at 05:11
  • 1
    @AliCan The filter function is also invoking `getB`. Assume that there are 100 elements and 10 are even. Since the `getB` is present in both `filter` and `map`, it would be invoked 100 + 10 times. But if you map and then filter, `getB` would be invoked only 100 times. – Gautham M Apr 07 '21 at 05:12
  • @ArvindKumarAvinash I tried to run with a static counter, for a list of 3 elements with 1 even number, the output of the counter value for the code in question was `4` and not `6`. – Gautham M Apr 07 '21 at 05:23
  • Thanks all, my bad. Indeed, it is obvious now. – Ali Can Apr 07 '21 at 05:31