4
\$\begingroup\$

I'm writting a registration feature currently and I I'm confused how to divide logic between layers. Can you review this code and tell if it's good way to do? In try-catch block I publish event to send verification email to new User.

  1. Should I keep in my Controller class just AuthService field? I'm using userRepository to check if username and email is available.

  2. In AuthService should I keep just authRepository and other services fields? Currently I'm using there RoleRepository.

  3. Is return type in my controller a good practise? ApiRespose is two-field class.

Controller:

@PostMapping("/signup")
public ResponseEntity<?> registerUser(@Valid @RequestBody SignUpRequest signUpRequest, HttpServletRequest httpServletRequest) {
    if (userRepository.existsByUsername(signUpRequest.getUsername())) {
        return new ResponseEntity(new ApiResponse(false, "Username is already in use!"), HttpStatus.BAD_REQUEST);
    }

    if (userRepository.existsByEmail(signUpRequest.getEmail())) {
        return new ResponseEntity(new ApiResponse(false, "Email is already in use!"), HttpStatus.BAD_REQUEST);
    }

    User user = new User(signUpRequest.getName(), signUpRequest.getUsername(), signUpRequest.getEmail(), signUpRequest.getPassword());

    User resultUser = authService.createUser(user);

    try {
        String host = httpServletRequest.getHeader("apphost");
        eventPublisher.publishEvent(new OnRegistrationCompleteEvent(resultUser, host));
    } catch (Exception e) {
        e.printStackTrace();
    }

    URI location = ServletUriComponentsBuilder
            .fromCurrentContextPath().path("/users/{username}")
            .buildAndExpand(resultUser.getUsername()).toUri();

    return ResponseEntity.created(location).body(new ApiResponse(true, "User registered successfully"));
}

Service:

@Override
public User createUser(User user) {
    user.setPassword(passwordEncoder.encode(user.getPassword()));

    Role userRole = roleRepository.findByName(RoleName.ROLE_USER)
            .orElseThrow(() -> new AppException("User Role not set."));

    user.setRoles(Collections.singleton(userRole));

    return userService.saveRegisteredUser(user);
}
\$\endgroup\$
1
\$\begingroup\$
  1. Should I keep in my Controller class just AuthService field? I'm using userRepository to check if username and email is available.

I think it is ok to use the UserRepository directly, but that is a matter of taste. You could create an additional UserService to wrap the calls to the UserRepository, or you could also create these wrapping methods in your AuthService.

Example:

public class UserService/AuthService {

   private UserRepository userRepository;

   public boolean userExistsByUsername(String username) {
       return userRepository.existsByUsername(username);
   }
}

Then you can use these UserService/AuthService methods in your controller.

For me, at this point, this looks like an unnecessary indirection, but this is the classic way of dividing layers.

  1. In AuthService should I keep just authRepository and other services fields? Currently I'm using there RoleRepository.

Same answer as for question 1. You could write a RoleService class which wraps the RoleRepository calls.

  1. Is return type in my controller a good practise? ApiRespose is two-field class.

I think that's totally ok, because you want to set different HTTP status codes.

Other things:

HTTP status code

new ApiResponse(false, "Username is already in use!"), HttpStatus.BAD_REQUEST);

Just a hint, the correct HTTP status code for existing entities is 409, so I would use HttpStatus.CONFLICT. But 400 is also ok.

Exception handling

This code

Role userRole = roleRepository.findByName(RoleName.ROLE_USER)
        .orElseThrow(() -> new AppException("User Role not set."));

is not inside a try-catch block so if this exception is not handled in another way by a @Provider or some @ExceptionHandler, you may improve this part. If you handling it globally, everything is fine.

| 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.