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.

• user9132502: Should i put it into a new class?
• K.H.: Honestly I can't answer that without knowing more context. One way would be to put it as protected into ancestor of this class or create a static function (and add parameter of type appUserService).
• K.H.: I expanded on this part of the answer a bit.

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);