2
\$\begingroup\$
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;
}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

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.
\$\endgroup\$
4
  • \$\begingroup\$ Should i put it into a new class? \$\endgroup\$ Oct 9, 2022 at 14:33
  • \$\begingroup\$ 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). \$\endgroup\$
    – K.H.
    Oct 9, 2022 at 15:58
  • \$\begingroup\$ I expanded on this part of the answer a bit. \$\endgroup\$
    – K.H.
    Oct 9, 2022 at 16:07
  • \$\begingroup\$ Perfect, thank you! \$\endgroup\$ Oct 9, 2022 at 17:05
3
\$\begingroup\$

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);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.