Duplicate code in spring controllers

Question

public ResponseEntity<Map<String, String>> resetPassword(@RequestBody String email) {
    // Create password token and send by email, if email doesn't exist, don't send it
    Optional<AppUser> optionalUser = appUserService.findByEmail(email);
    if (optionalUser.isEmpty()) {
        throw new VerificationTokenException(HttpStatus.UNAUTHORIZED, "Email not found");
    }

    AppUser user = optionalUser.get();
    if (user.isEnabled()) {
        throw new VerificationTokenException(HttpStatus.FORBIDDEN, "Something went wrong");
    }

    Optional<VerificationToken> optionalVerificationToken = verificationTokenService.findByUser(user);
    if (optionalVerificationToken.isPresent()) {
        VerificationToken verificationToken = optionalVerificationToken.get();
        verificationTokenService.delete(verificationToken);
    }
    
    // ...
}

These 3 if staments are used in multiple methods in my Controller, what is a proper way to make them reusable?

I could extract each if statement into an individuel method like so, but on what level should I put them in? Should I create an util class or keep them inside my Controller class which uses them?

private Optional<AppUser> getAppUser(String email) {
    Optional<AppUser> optionalUser = appUserService.findByEmail(email);
    if (optionalUser.isEmpty()) {
        throw new VerificationTokenException(HttpStatus.UNAUTHORIZED, "Email not found");
    }
    return optionalUser;
}

Answer 1

You can just extract those first 2 if's into separate methods (or single method), returning the user at the same time. Something like getDisabledUserOrThrow() (Returning the AppUser instance, not Optional, because either you return a "valid" user (no-enabled) or throw your exception).

You can use that anywhere you need to ensure you have user instance. Something like this:

private AppUser getDisabledUserOrThrow(String email) throws VerificationTokenException {
    Optional<AppUser> optionalUser = appUserService.findByEmail(email);
    if (optionalUser.isEmpty()) {
        throw new VerificationTokenException(HttpStatus.UNAUTHORIZED, "Email not found");
    }

    AppUser user = optionalUser.get();
    if (user.isEnabled()) {
        throw new VerificationTokenException(HttpStatus.FORBIDDEN, "Something went wrong");
    }
    return user;
}

You could do the same to get the token if that's what is the most common data necessary for your logic further.

Edit:

To expand on positioning this method:

• One way would be to put it in your parent controller (if you have one) as protected methods so that you can call them directly from your controller.
• I am also thinking - if those methods are basically getting your domain models, you can look at those as methods of Repositories (look up this pattern if needed) and inject them into your controllers :-)
• It's always possible to create static methods instead, which is also fine if there's no clear place of putting it.

Answer 2

I'd like to point out that you are using Optional badly. Generally the use of isPresent/isEmpty and get are an anti-pattern. Instead in this case you should be using orElseThrow and ifPresent, for example like this:

AppUser user = appUserService.findByEmail(email).orElseThrow(
    () -> new VerificationTokenException(HttpStatus.UNAUTHORIZED, "Email not found")
);

if (user.isEnabled()) {
    throw new VerificationTokenException(HttpStatus.FORBIDDEN, "Something went wrong");
}

verificationTokenService.findByUser(user).ifPresent(verificationTokenService::delete);