-2
\$\begingroup\$

Is there any way to clean code with Optional or something else?

public Token getValidUserToken(User user) {
    final Optional<Token> token = tokenRepository.getTokenByUser(user.getEmail());
    Token result;
    if (token.isPresent()) {
        result = token.get();
        if (result.isExpired()) {
            tokenRepository.expireToken(result.getToken());
        }
    } else {
        result = generator.generateNewToken(user);
    }

    result = tokenRepository.save(result);
    Hibernate.initialize(result.getUser());

    return result;
}
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

Indeed, there are some possible improvements for the instructions that handle Optional: instead of creating conditions with isPresent(), the style is more "functional" when orElseGet() (or other orElse* methods) are called.

For example:

public Token getValidUserToken(User user) {
  final Token token = tokenRepository.getTokenByUser(user.getEmail())
                                     .orElseGet(() -> generator.generateNewToken(user));
  if (token.isExpired()) {
    tokenRepository.expireToken(token.getToken());
  }
  final Token saved = tokenRepository.save(token);
  Hibernate.initialize(saved.getUser());
  return saved;
}

Here, the check with isExpired is placed outside the retrieval sequence and I assume that this condition is false for a new created token.

\$\endgroup\$
2
  • \$\begingroup\$ I tried this, but this is not valid because I need compare seconds. Anything else? \$\endgroup\$ Commented Jun 28, 2017 at 8:11
  • 2
    \$\begingroup\$ @EugeneG.Ustimenko you need "compare seconds" ? what does that even mean ? if you need to use isExpired only on token retrieved from tokenRepository, then consider using filter from optional. Great answer BTW \$\endgroup\$ Commented Jun 28, 2017 at 9:42
0
\$\begingroup\$

Do not use Optional as a general substitute for null.

In your case it is absolutly ok to not use Optional. You will have less noise in your code.

Optional is a way to express, that your code should not depend on a value. As you create the value right after you determined it is not present your algorithm seems to have not an "Option" to bypass the non-existence.

Your algorithm is a lazy initialization mechanism where a null check is appropriate. Everything else is noise and should be omitted.

There are rare occasions to use Optional.

Your result value is not optional. The using code depends on the existence of a non null value. Using Optional is misleading.

My bigger concern is that hibernate initialization mechanism. As I do not know your other code I don't know if it is neccessary. But this statement is a surprise. It does some magic to your structure to return AND it is directly dependent of a concrete technology instead of an abstraction like JPA. I highly recommend to avoid such mechanisms and adress this kind of micro management architectural. But as I do not have any further code I cannot make detailed suggestions on that.

\$\endgroup\$
1
  • \$\begingroup\$ @Query("SELECT t from Token t WHERE lower(t.user.email)=lower(:email) AND t.expiredDate = (SELECT MAX(tt.expiredDate) FROM Token tt WHERE tt.user.id=t.user.id)") Optional<Token> getTokenByUser(@Param("email") String email); \$\endgroup\$ Commented Jun 28, 2017 at 8:17

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.