Dfyjkt

I've started to use Java's 8 Optionals. and I would like to share this method, it is "code smells" example and I would like to rewrite it using java 8 and optionlas and functional (declarative) style, so I am interested in seeing your opinion on it. Let's consider this method:

 public boolean isTokenValid(String userAgent, Optional<String> apiKey) 
 LOGGER.info(String.format("userAgent : %s, Key ?: %s", userAgent, apiKey.isPresent()));
 if ("ALFA".equalsIgnoreCase(userAgent))
 return (apiKey != null && apiKey.isPresent()
 && ispropertyExists(ALFA_TYPE, apiKey.get()));
 
 else 
 return (apiKey != null && apiKey.isPresent()
 && ispropertyExists(BETA_TYPE, apiKey.get()));
 

Where "ispropertyExists" returns boolean type, and "ALFA_TYPE" and "OMEGA_TYPE" are enums constants.
So below is the way I rewrote this method in intention to improve the readability and practice functional thinking style. I've added comments, to explain my thoughts and reasons I did so and so, I appreciate your opinions and examples of your ways if you think you able to improve it.

 /**
 * We should not pass Optionals as a parameters to the methods. We
 * should use Optionals only for return value when we are not sure if value will
 * be presented at the end of the calculations or not.
 */
public boolean isTokenValid(String userAgent, String apiKey) 
 LOGGER.info(String.format("userAgent : %s, Key ?: %s", userAgent, apiKey));

 /**
 * If apiKey is null then it is incorrect. And execution will stop after
 * Optional.ofNullable(apiKey), since monad will be having null value. If apiKey
 * is not null then we still want to filter out empty strings. If after filter
 * there will be no value, then execution will stop.
 * If we have some value for apiKey then it is ok and we map the monad to the
 * userAgent value to proceed the chain of calls on monad.
 */
 Optional<String> userAgentOptional = Optional.ofNullable(apiKey).filter(StringUtils::isNotBlank)
 .map(ak -> userAgent);

 /**
 * We map "userAgent" value to boolean (if it is a alfa or not). Then
 * we map that boolean to boolean value which represents security check in db
 * itself.
 */
 Optional<Boolean> isValid = userAgentOptional.map(ua -> "ALFA".equalsIgnoreCase(ua))
 .map(isAlfa -> isAlfa ? ispropertyExists(ALFA_TYPE, apiKey)
 : ispropertyExists(BETA_TYPE, apiKey));

 /**
 * And all in all we get value from our optional boolean. If "somehow" it is
 * ended up to be empty, then we retrun "false", if it is not empty, then the
 * value will itself be returned.
 */
 return isValid.orElse(false);

Thank you.

I would combine all the operations in one chained statement and return the result avoiding unnecessary Optional variables.

 return Optional.ofNullable(apiKey)
 .filter(StringUtils::isNotBlank)
 .map(ak -> userAgent)
 .map("ALFA"::equalsIgnoreCase)
 .map(isAlfa -> isAlfa ? ALFA_TYPE : BETA_TYPE)
 .map(type -> ispropertyExists(type, apiKey))
 .orElse(false);

IMHO you’re overdoing and overcomplicating it. I agree that in general we should not pass an Optional as a parameter to a method. If you cannot require the passed apiKey to be non-null, my suggestion would be:

public boolean isTokenValid(String userAgent, String apiKey) 

I would find this distinctively simpler. There is no need to use an Optional for your case.

If you love functional style, first try to not use null, or at least don’t pass null around.

But if you must use null, here is my code for you:

public boolean isTokenValid(String userAgent, String apiKey) 
 final Enum type = "ALFA".equalsIgnoreCase(userAgent) ? ALFA_TYPE : BETA_TYPE;
 return
 Optional.ofNullable(apiKey)
 .filter(ak -> ispropertyExists(type, ak))
 .isPresent();

PS: functional style doesnt mean trying to put everything chained and avoid temporary values. Rather, it is about the use of pure functions and immutable data. Regardless of style, our goal is to write readable and reasonable code. Pure functions and immutable data are very suitable for that goal.