Why can't I use filter as my last step in a stream

Question

I keep getting told it is bad practice to not terminate a Stream via methods such as collect and findFirst but no real feedback as to why not much said about it in blogs.

Looking at following example, instead of using a massive nested if check, I went with Optional to get back a List value. As you can see my last step is filter in that Stream. This works as expected for me which is to get back a list. Why is this wrong and how should I have written it instead?

import lombok.Getter;
import lombok.Setter;
import java.util.*;

public class Main {
    public static void main(String[] args) {

        RequestBean requestBean = new RequestBean();
        // if I uncomment this I will get the list values printed as expected
//        FruitBean fruitBean = new FruitBean();
//        AnotherBean anotherBean = new AnotherBean();
//        InnerBean innerBean = new InnerBean();
//        requestBean.setFruitBeans(Collections.singletonList(fruitBean));
//        fruitBean.setAnotherBeans(Collections.singletonList(anotherBean));
//        anotherBean.setInnerBeans(Collections.singletonList(innerBean));
//        List<String> beans = Arrays.asList("apple", "orange");
//        innerBean.setBeans(beans);

        List<String> result = getBeanViaOptional(requestBean);

        if(result != null){
            for(String s : result){
                System.out.println(s);
            }
        }else {
            System.out.println("nothing in list");
        }

    }

    private static List<String> getBeanViaOptional(RequestBean bean){
        Optional<List<String>> output = Optional.ofNullable(bean)
                .map(RequestBean::getFruitBeans)
                .map(n -> n.get(0))
                .map(FruitBean::getAnotherBeans)
                .map(n -> n.get(0))
                .map(AnotherBean::getInnerBeans)
                .map(n -> n.get(0))
                .map(InnerBean::getBeans)
                // why is this bad practice to end with a filter. how should I write this then?
                .filter(n -> n.contains("apple"));

        if(!output.isPresent()){
            throw new CustomException();
        }

        return output.get();
    }

    // not using this. just to show that optional was preferable compared to this.
    private static List<String> getBeanViaIfChecks(RequestBean bean){
        if(bean != null){
            if(bean.getFruitBeans() != null){
                if(bean.getFruitBeans().get(0) != null){
                    if(bean.getFruitBeans().get(0).getAnotherBeans() != null){
                        if(bean.getFruitBeans().get(0).getAnotherBeans().get(0) != null){
                            if(bean.getFruitBeans().get(0).getAnotherBeans().get(0).getInnerBeans() != null){
                                if(bean.getFruitBeans().get(0).getAnotherBeans().get(0).getInnerBeans().get(0) != null){
                                    return bean.getFruitBeans().get(0).getAnotherBeans().get(0).getInnerBeans().get(0).getBeans();
                                }
                            }
                        }
                    }
                }
            }
        }
        return null;
    }
}

@Getter
@Setter
class RequestBean{
    List<FruitBean> fruitBeans;
}

@Getter
@Setter
class FruitBean{
    List<AnotherBean> anotherBeans;
}

@Getter
@Setter
class AnotherBean{
    List<InnerBean> innerBeans;
}

@Getter
@Setter
class InnerBean{
    List<String> beans;
}

class CustomException extends RuntimeException{
    // do some custom exception stuff
}

But this `filter` method comes from `Optional` and not `Stream`. — Michał Krzywański, Sep 21 '19 at 11:06
I am not aware why/when such use cases of `get(0)` occurs and while I am not certain of a cleaner way to avoid them. They do itch my eyes while reading a functional approach. — Naman, Sep 21 '19 at 13:31
Another point to notice between the two approaches is that the traditional `for` loop approach is not equivalent to the functional approach since it doesn't ever throw a `CustomException`. — Naman, Sep 21 '19 at 13:42
@Naman The response is coming from another team's micro service in the company's structure which provides this response format where there can only be one element in those list thus going for .get(0). (As to why they decided to go with a list, I have no idea). — kar, Sep 21 '19 at 13:46

score 10 · Accepted Answer · edited Sep 21 '19 at 13:04

I keep getting told it is bad practice to not terminate a Stream via methods such as collect and findFirst but no real feedback as to why not much said about it in blogs.

It really depends on the context, if you're saying "can I end a stream with an intermediate operation e.g. filter and not call a terminal operation (an operation that consumes the stream) ever" then yes it's bad practise and kind of pointless because you've just defined some criteria but never asked for the "result".

Streams are lazy in the sense that they don't do anything unless told so by a terminal operation e.g. collect, findFirst etc.

If you're saying "is it bad practice to return a stream from a method" then it may be worth reading this answer on whether one should return a stream or a collection.

Further, note that your getBeanViaOptional logic is operating on an Optional<T> rather than a Stream<T>. Yes, they both have map, flatMap and filter but note that an Optional<T> can only contain one value or it's empty whereas a stream can have one or more.

Your approach of using an Optional instead of the imperative ifs is obviously better in terms of readability, maintenance, etc. so I'd suggest you proceed with that approach, although you can improve it a little bit by using orElseThrow i.e.:

return Optional.ofNullable(bean)
               .map(RequestBean::getFruitBeans)
               .map(n -> n.get(0))
               .map(FruitBean::getAnotherBeans)
               .map(n -> n.get(0))
               .map(AnotherBean::getInnerBeans)
               .map(n -> n.get(0))
               .map(InnerBean::getBeans)
               .filter(n -> n.contains("apple"))
               .orElseThrow(CustomException::new);

Donat · Answer 2 · 2020-01-17T15:20:27.963

7

With streams usually none of the intermediate operations will be executed when there is no terminal operation. Your example uses Optional. Its operations map and filter have the same name as some intermediate operations in stream, but they are different. Your example is ok (not bad practice) at the line asked by your question.

Another thing is that (as already pointed out by Aomine) .orElseThrow is the shorter way to get the value in the Optional and throw an exception if there is none. Even more important is that it is safer to use .orElseThrow (or .orElse if there is a default value). Optional.get() should be avoided whenever possible. You will get a NoSuchElementException if there is no value. This is almost as bad as getting a NullPointerException when not using Optional. Optional used in a proper way can protect you from NullPointerException.

edited Jan 17 '20 at 15:20

answered Sep 21 '19 at 11:16

Donat

4,157
3
11
26

1

"`....Optional.get()` should be avoided whenever possible" true but if you're checking for the presence of the value in the optional prior to calling `get` as OP has done then it's completely safe; but as you've hinted... there is almost always a better way to get a value out of an optional without using `get`. – Ousmane D. Sep 21 '19 at 16:44
1

Yes, it is safe if you check for presence before using ```.get()```, but it is easy to forget and hard to check in a big code base, if you often use ```.get()```. The other methods for getting the value make syntactically sure that there is some handling in case of absence. This is also the advantage over using ```null```. It is safe to access a variable that maybe ```null```, if you check before. But it is easy to forget and hard to check that it has not been forgotten in some places. – Donat Sep 21 '19 at 18:27

Why can't I use filter as my last step in a stream

2 Answers2