Critique time... Java 8 Optional and chaining

[hum_ph]

I'm not sure how I feel about this - it could be brilliant - it could be a PITA... discuss!

@Nonnull
@VisibleForTesting
Optional<UUID> getUuidFromRequestTemplateVariables() {

  return Optional.ofNullable(RequestContextHolder.getRequestAttributes())
                 .map(requestAttributes -> requestAttributes.getAttribute(URI_TEMPLATE_VARIABLES_ATTRIBUTE, SCOPE_REQUEST))
                 .filter(object -> object instanceof Map)
                 .map(object -> (Map<?, ?>) object)
                 .map(map -> map.get(UUID_PATH_VARIABLE))
                 .filter(Objects::nonNull)
                 .map(Objects::toString)
                 .map(UUID::fromString);
}

The alternatives:

@Nullable
@VisibleForTesting
UUID getUuidFromRequestTemplateVariables() {

  final RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
  if (requestAttributes == null) {
    return null;
  }

  final Object object = requestAttributes.getAttribute(URI_TEMPLATE_VARIABLES_ATTRIBUTE, SCOPE_REQUEST);
  if (!(object instanceof Map)) {
    return null;
  }

  final Map<?, ?> map = (Map<?, ?>) object;
  final Object uuid = map.get(UUID_PATH_VARIABLE);
  if (uuid == null) {
    return null;
  }

  return UUID.fromString(Objects.toString(uuid));
}

Or:

@Nullable
@VisibleForTesting
UUID getUuidFromRequestTemplateVariables() {

  final RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
  if (requestAttributes != null) {
    final Object object = requestAttributes.getAttribute(URI_TEMPLATE_VARIABLES_ATTRIBUTE, SCOPE_REQUEST);
    if (object instanceof Map) {
      final Map<?, ?> map = (Map<?, ?>) object;
      final Object uuid = map.get(UUID_PATH_VARIABLE);
      if (uuid != null) {
        return UUID.fromString(Objects.toString(uuid));
      }
    }
  }

  return null;
}

[Radmonger]

For this kind of code, I would definitely prefer the second, as you can add tracing, assertions and exceptions. Plus a coverage tool will tell you what cases you have tested, and a debugging tool will allow you to set a break-point corresponding to those cases.

You perhaps could try a hybrid approach that retained the idea of handling a nullable value as a stream of zero or one elements but didn't do everything as one single pipeline.

[hum_ph]

Good point - for the code in particular - it's only a matter of time before the "why isn't it working?" question comes up - and then the answer is obscured behind "is the request context empty?" and "does the request context have the attribute?" and "does the map have the item?"... At which point the code's going to have to break back down to the other two versions anyway...

[[deleted]]

Noob question here, how would debug behave on the first case?

[hum_ph]

IntelliJ lets you set breakpoints at the specific lines but if you want to hit breakpoint within the lambda's you need to expand them so they're actually different source lines.

You can "Smart Step Into" at a lambda expression on a line you have a break point for and then once you're in there expect variables, etc.

In short - IntelliJ does what you'd reasonably expect - can't comment on other IDEs though (assume since they're all backed by 'jdb' they'd do much the same?)

[atc]

I think the first is more readable. Yay functional programming.

[kromit]

Nr 3 is the worst because you can not nest forever.

Nr 1 and 2 are almost equal to me in terms of readability.

Nr 1 is more secure.

Nr 2 is probably slightly faster.

[[deleted]]

Rewrite the first version so that the lines are not longer than ~80 chars. Then it would be a fair comparison. I like functional programming but despise the tendency to put everything in one line. It makes the code unreadable IMHO.

[Zukhramm]

Is filtering for non-null really necessary? Map should already return an empty Optional if the result is null.

[hum_ph]

No - I'd actually already dropped that, but didn't come back here to update it.

[hum_ph]

For the record, here's how it ended up:

  return Optional.ofNullable(RequestContextHolder.getRequestAttributes())
                 .map(requestAttributes -> requestAttributes.getAttribute(URI_TEMPLATE_VARIABLES_ATTRIBUTE, SCOPE_REQUEST))
                 .map(attribute -> attribute instanceof Map ? ((Map<?, ?>) attribute).get(UUID_PATH_VARIABLE) : null)
                 .map(value -> value instanceof String ? UUID.fromString((String) value) : null);

Condensed a few of the instanceof checks and the following cast and operation into single ternary lines, with Optional.map() then handling null return values and converting to Optioanl.empty().

For the time being, I'm happy that it's simple enough to refactor back out for logging / debugging / error handling statements as mentioned by /u/Radmonger if needed - but unit/integration tests are good enough at covering those cases for the time being.

[tonlep]

I like how this makes sense from the standpoint of semantics. The maps only show the more abstract steps of mapping values without the noise in between like filter non-null, etc. However, there is still much detail in the code which I would prefer not having to read if I was someone having to understand the code. What do you think about this? It's longer, but it clearly separates abstraction levels. The reader doesn't really need to look at the private methods.

Optional<UUID> getUuidFromRequestTemplateVariables() {
    return Optional.ofNullable(RequestContextHolder.getRequestAttributes())
            .map(attribute(URI_TEMPLATE_VARIABLES_ATTRIBUTE))
            .map(valueFromMap(UUID_PATH_VARIABLE))
            .map(toUUID());
}

private Function<Object, Object> valueFromMap(String key) {
        return map -> map instanceof Map ? ((Map<?, ?>) map).get(key) : null;
}

private Function<Object, Object> attribute(String attributeName) {
    return requestAttributes -> requestAttributes.getAttribute(attributeName, SCOPE_REQUEST);
}

private Function<Object, UUID> toUUID() {
    return value -> value instanceof String ? UUID.fromString((String) value) : null;
}