1
\$\begingroup\$

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?

\$\endgroup\$
  • 3
    \$\begingroup\$ Why is userIds nullable? Passing in an empty set, instead of null, would be equivalent, and cleaner, and remove the Optional.ofNullable(). \$\endgroup\$ – AJNeufeld Sep 2 at 3:36
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ the findAll return an iterable so there is no stream, I m ok with the anti-patern you r right and I would remove it, \$\endgroup\$ – Mohammed Housseyn Taleb Sep 3 at 16:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.