Is there better alternative to making an optional set of elements from iterable in java?

Question

I'm feeling uneasy with this service method that I wrote:

    @Override
    public Optional<Set<TenantDTO>> getAllTenantsOfUsers(Set<Long> usersIds) {

        return Optional.ofNullable(usersIds)
                .map((ids) -> tenantRepository.findAll(QTenant.tenant.users.any().id.in(usersIds)))
                .map((it) -> StreamSupport.stream(it.spliterator(), false))
                .map((strm) -> strm.map((tenant) -> tenantMapper.toDto(tenant)).collect(Collectors.toSet()));

    }

My tenantRepository.findAll returns an iterable of Tenant entity

Is this the better approach to get all performance, with good code?

Answer 1

Both optional collection and a collection of optionals are anti-patterns. There is no scenario where returning Optional<Set<X>> or Set<Optional<X>> is preferable to returning a Set<X> that can be empty.

As @AJNeufeld mentions, a guard condition is a good way to start. In your case. If incoming usersIds are null then return an empty set.

public Set<TenantDTO> getAllTenantsOfUsers(Set<Long> usersIds) {
    if (usersIds == null) {
        return Collections.emptySet();
    }
    // ...
}

Its really confusing when you mix Optional.map and Stream.map in a single call chain. But since the usersIds == null case is handled, you don't need Optional.ofNullable anymore. You can simplify this to just operations over your ids and Tenants.

Something like this:

public Set<TenantDTO> getAllTenantsOfUsers(Set<Long> usersIds) {
    if (usersIds == null) {
        return Collections.emptySet();
    }
    return tenantRepository
            .findAll(QTenant.tenant.users.any().id.in(usersIds))
            .stream()
            .map((tenant) -> tenantMapper.toDto(tenant))
            .collect(Collectors.toSet());
}

Now that getAllTenantsOfUsers returns Set(possibly empty Set), all clients of this code can use it directly instead of caring about handling and unwrapping optional.