2
\$\begingroup\$

In my Spring MVC app I have User and Role entities.

User:

public class User {
    private Long id;
    private Set<Role> roles;

    // other fields, getters and setters
}

Role:

public class Role {
    private Long id;

    // other fields, getters and setters
}

I have a service method with the following signature:

@Transactional
void setRolesByUserId(Map<Long, Set<Long>> rolesByUserId)

The key of the Map is a User ID and the corresponding value is a Set of Role IDs that needs to be assigned to the user.

My implementation:

public void setRolesByUserId(Map<Long, Set<Long>> rolesByUserId) {
    // 1.
    Map<Long, Role> roles = roleRepository.findAll()
        .stream().collect(Collectors.toMap(Role::getId, Function.identity()));

    // 2.
    userRepository.findAllById(rolesByUserId.keySet())
            // 3.
            .forEach(user -> user.setRoles(rolesByUserId.get(user.getId())
                    .stream().map(roles::get).collect(Collectors.toSet())));
}

So basically the flow is the following:

  1. fetch all the Role entities from the DB into a Map<Long, Role> (where the key is the Role ID and the value is the Role entity itself)
  2. fetch the User entities from the DB that needs to be modified
  3. for each User, iterate through the Role IDs to be assigned, fetch the corresponding Role entities from the Map, collect them into a Set and assign it to the User

Can this method be optimized?

\$\endgroup\$
1
\$\begingroup\$

The biggest optimization potential I see is readability. Note that forEach() does not have any advantage over a good-ole-for-loop from a technical perspective, but in many cases makes the code more complicated, especially when the lambda is not trivial.

Thus, I'd replace the steps 2 and 3 with the following equivalent code:

for(User user : userRepository.findAllById(rolesByUserId.keySet())) {
    Set<Roles> roles = rolesByUserId.get(user.getId()).stream()
        .map(roles::get)
        .collect(Collectors.toSet());
    user.setRoles(roles);
}
  • it is absolutely clear, that you iterate over user objects
  • the roles are plainly visible
  • no nested streaming or lambdas
  • and you can set a breakpoint at various places to inspect the current user and role-set in a debugger
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Well, you created a good code so far. Your lamda is OK. But there are some things about naming which could be improved

  1. About the API setRolesByUserId, it is OK to accept userId as long. But about role, it could be better to change the Set to Set, and i think you're about to update for List of user, hence the name should change to updateUserWithRoleSet.
  2. IMHO, It is totally OK to update one user at once => updateUserWithRole(long userId, Set<Role> roles>
| improve this answer | |
\$\endgroup\$

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.