How can I split this function for low congnitive complexity?

Question

I'm new to Java and this noob function has been bothering me. I have downloaded sonarLint and lo and behold it throws this issue on my face Cognitive Complexity of methods should not be too high. I know it looks ugly but can anyone point out how I can reformat to have dry concept and also not have high complexity as mentioned by the SonarLint.

@PostMapping("/adduser")
//  @PreAuthorize("hasRole('ADMIN')")
    public ResponseEntity<MessageResponse> registerUser(@Valid @RequestBody SignupRequest signUpRequest) {
        /*
         * This controller Creates new user based on all the entities for the user
         * 
         */
         if(dbValue)
         {
             if (repository.existsByUsername(signUpRequest.getUsername())) {
                    return ResponseEntity
                            .badRequest()
                            .body(new MessageResponse("Error: Username is already taken!"));
                }

                if (repository.existsByEmail(signUpRequest.getEmail())) {
                    return ResponseEntity
                            .badRequest()
                            .body(new MessageResponse("Error: Email is already in use!"));
                }

                // Create new user's account
                User2 user = new User2(signUpRequest.getUsername(), 
                                     signUpRequest.getEmail(),
                                     signUpRequest.getCustomername(),
                                     signUpRequest.getCustomerid(),
                                     signUpRequest.getDescription(),
                                     encoder.encode(signUpRequest.getPassword()));              
                
                Set<String> strRoles = signUpRequest.getRoles();                
                Set<Role2> roles = new HashSet<>();
                Role2 e = new Role2();
                e.setName("ROLE_ADMIN");
                roles.add(e);               
                user.setRoles(roles);
                repository.save(user);      
                
         }
         
         else {
             
             if (repository.existsByUsername(signUpRequest.getUsername())) {
                    return ResponseEntity
                            .badRequest()
                            .body(new MessageResponse("Error: Username is already taken!"));
                }

                if (repository.existsByEmail(signUpRequest.getEmail())) {
                    return ResponseEntity
                            .badRequest()
                            .body(new MessageResponse("Error: Email is already in use!"));
                }

                // Create new user's account
                User1 user = new User1(signUpRequest.getUsername(), 
                                     signUpRequest.getEmail(),
                                     signUpRequest.getCustomername(),
                                     signUpRequest.getCustomerid(),
                                     signUpRequest.getDescription(),
                                     encoder.encode(signUpRequest.getPassword()));
                

                Set<String> strRoles = signUpRequest.getRoles();
                Set<Role> roles = new HashSet<>();
              
                if (strRoles == null) {
                    Role userRole = roleRepository1.findByName(URole.ROLE_USER)
                            .orElseThrow(() -> new RuntimeException("Error: Role is not found."));
                    roles.add(userRole);
                } else {
                    strRoles.forEach(role -> {
                        switch (role) {
                        case "admin":
                            Role adminRole = roleRepository1.findByName(URole.ROLE_ADMIN)
                                    .orElseThrow(() -> new RuntimeException("Error: Role is not found."));                          
                            roles.add(adminRole);

                            break;
                        
                        default:
                            Role userRole = roleRepository1.findByName(URole.ROLE_USER)
                                    .orElseThrow(() -> new RuntimeException("Error: Role is not found."));
                            roles.add(userRole);
                        }
                    });
                }
                user.setRoles(roles);
                repository.save(user);
         }      
         return ResponseEntity.ok(new MessageResponse("User Added successfully!"));     
    }   

Appreciate criticism and any help. Thanks in Advance.

Answer 1

1. In the controller, you need to work with a service, not a repository. Make user creation method there.
2. Create interface UserValidator with method (existsByUsername, existsByEmail implements predicate) Optional check(Predicate condition, String message) OR delegate this methods to a new validation utility class.
3. You have dublicate, may move it outside the condition

if (repository.existsByUsername(signUpRequest.getUsername())) 
    return ResponseEntity
        .badRequest() // below, you always create a new string and a new message response, use a constant as not to clog the memory
        .body(new MessageResponse("Error: Username is already taken!"));
if (repository.existsByEmail(signUpRequest.getEmail())) 
    return ResponseEntity
        .badRequest() // here too
        .body(new MessageResponse("Error: Email is already in use!"));

4. signUpRequest.getRoles() may returns an Optional, or Collections.emptyList() if an empty, not a null
5. Get role by .valueOf(), it make .forEach() conditions unused and switch-case too.
6. Replace this

Role userRole = roleRepository1.findByName(URole.ROLE_USER)
   .orElseThrow(() -> new RuntimeException("Error: Role is not found."));
roles.add(userRole);

with

roleRepository1.findByName(URole.ROLE_USER).ifPresent(roles::add);

Answer 2

Just look to apply Extract Method consistently and it will greatly improve your code. Doing so will also help you to discover responsibilities and pave the way for further refactoring.

You could take your existing code as is and just split it to smaller self-descriptive methods to make the main algorithm much more understandable.

E.g.

if (repository.existsByUsername(signUpRequest.getUsername())) {
    return usernameTakenError();
}
if (repository.existsByEmail(signUpRequest.getEmail())) {
    return emailUsedError();
}
user = userToSignup(signUpRequest);
userRepository.save(user);
return ResponseEntity.ok(new MessageResponse("User Added successfully!")); 

At this point it's much less important how those methods are actually implemented: the core algorithm remains short & understandable.

Note that I often convert exceptions to response errors automatically which allows to check rules in a declarative way e.g.

//throws if taken and a generic exception handler 
//returns and error response
assertUsernameFree(); 

Also look for DRY opportunities. For instance, rather than:

roleRepository1.findByName(role).orElseThrow(() -> new RuntimeException("Error: Role is not found."));

You could have roleRepository1.roleOfName(role) or findExistingByName which throws instead of returning Optional.

Furthermore, the whole roles mapping section of your code is way too complex and I think there might be a bug in there: the default switch case where any unknown role is added as USER_ROLE doesn't really seem to make any sense to me.

I'd expect something more along the lines of (which would be in a resolveSignupRoles function or even double-dispatch signUpRequest.getRoles(rolesRepo)):

Set<Role> roles = signUpRequest.getRoles().stream()
    .map(r -> URole.fromCode(r))
    .map(r -> userRepository::roleOfName).collect(Collectors.toSet());

if (roles.isEmpty()) roles.add(userRepository.roleOfName(URole.ROLE_USER));

There are tons of refactoring opportunities here, but Extract Method is easy to apply and will immediately improve all the code you write.