Combine the Two Lines of code initializing and populating a List into a Single Statement in Java 8?-CodePudding

I have a list of user id pmUserIds. For each id in the list I'm making a call getUserByUserId() and placing the result in a list of type List<Optional<User>>.

I want to combine these 2 lines into a single statement:

List<Optional<User>> pmUsers = new ArrayList<>();

pmUserIds.forEach(userId -> pmUsers.add(userRepository.getUserByUserId(userId)));

Something like that wouldn't work:

List<Optional<User>> pmUsers  = pmUserIds.forEach(userId -> pmUsers.add(userRepository.getUserByUserId(userId)));

It will say that pmUsers not have been initiated. Is there any way we could do this in single line?

CodePudding user response：

Use Stream.map instead of forEach, then convert to List with collect:

List<Optional<User>> pmUsers = pmUserIds.stream()
        .map(userId -> userRepository.getUserByUserId(userId))
        .collect(Collectors.toList());

That's still not one line (except you want a really long line), but it's somewhat clearer what the code does that with separate initialization and forEach. You may also use a method reference instead of that lambda, i.e. .map(userRepository::getUserByUserId)

CodePudding user response：

Your second attempt is not viable because method forEach() is void. To do the job in a single statement, you need a Stream.

But there is more to it.

Storing Optional objects into a Collection is an antipattern. It almost the same as storing null references because optional not necessarily contains a value.

Both null and empty optional are meant to represent the absence of data and nothing else. If one of them has a separate meaning in your application (apart from "no value"), it's a smell.

If you need to fire a particular action when a call getUserByUserId() returns an empty optional, then do it right after receiving the optional instead of placing it to the list (and by the way, Stream is not the right tool when you need to perform some side-effects along the way, it would be better to stick with plain loops).

Here's a small quote from the answer by @Stuart Marks, Java and OpenJDK developer:

I'm sure somebody could come up with some contrived cases where they really want to store an Optional in a field or a collection, but in general, it is best to avoid doing this.

I suggest you to check the presence of value in the Optional right on the spot (in the stream) and then "unpack" non-empty optional objects.

That's how it might look like:

List<User> pmUsers  = pmUserIds.stream()
    .map(userRepository::getUserByUserId) // Stream<Optional<User>>
    .filter(Optional::isPresent)
    .map(Optional::get)                   // Stream<User>
    .toList(); // for Java 16  or .collect(Collectors.toList()) for earlier versions