Getting a value from an inside an Optional with isPresent()-CodePudding

I have a User and associated time-limited Roles. I want to know if a User has a particular UserRole and it is unexpired. I can turn the user's roles into a stream, filter() it and findFirst(), giving me an Optional.

Role

public class Role {
    private UserRole role;
    private Date expiry;
    
    public boolean isUnexpired () {
        return (expiry == null) ? true : expiry.after(new Date());
    }
}

User

public class User {
  //...
  private Collection<Role> roles

  public boolean hasRole (UserRole userRole) {
    return roles.stream()
      .filter(r -> r.getRole().equals(userRole))
      .findFirst()
      .ifPresent(ur -> {  /* ... herein the problem ... */ ur.isUnexpired(); } );
  }
}

The problem in that last line is that ifPresent() has a void signature; as such, I can't return ur.isUnexpired() from it. Whatever I put in the lambda expression or anonymous inner class at that point can't do anything meaningfully with the value it finds.

I tried declaring a boolean before filtering the stream, and assigning it, but get the (code validation) error: local variables referenced from a lambda expression must be final or effectively final.

(I know, there's a bit more to handle when it's not present; if I can sort this, I can swap to ifPresentOrElse().)

I could do the following:

  public boolean hasRole (UserRole userRole) {
    Optional<Role> o = roles.stream()
      .filter(r -> r.getRole().equals(userRole))
      .findFirst();
    return o.isPresent() ? o.get().isUnexpired() : false;
  }

However, I would rather do it with a cleaner, chained function.

Is there some way to extract and use my isUnexpired() boolean with a chained function? Or must I assign the Optional then operate on it separately?

CodePudding user response：

You should use Optional::map to retrieve value of isUnexpired, and orElse to return false:

public boolean hasRole (UserRole userRole) {
    return roles.stream()
        .filter(r -> r.getRole().equals(userRole))
        .findFirst()
        .map(Role::isUnexpired)
        .orElse(false);
}

CodePudding user response：

However, I would rather do it with a cleaner, chained function.

Why would a chained function be 'cleaner'? As you can see, it makes it much more difficult to adapt code to changing requirements, and enforces weird style choices on your other code in order to dance around the fact that you can't use mutable local variables and don't get control flow or checked exception transparency. I don't know what definition of 'cleaner' you're working on, but clearly it's not "code that leads to easier to modify, easier to test, easier to read, easier to fit inside code with other requirements", which seems like a much more sensible definition to me. Perhaps your definition is based in aesthetics. Well, as they say, you can't argue with taste, perhaps.

At any rate, you have two options:

Map optional.NONE onto a sentinel that then fails the test.

You can simply use .orElse() here:

...
.findFirst()
.orElse(dummyRoleThatIsDefinitelyExpired)
.isUnexpired();

Map the optional.SOME

...
.findFirst()
.map(r -> r.isUnexpired())
.orElse(false);

The map call will return either an Optional.NONE, or an Optional of Boolean.TRUE or an optional of Boolean.FALSE. We then orElse the NONE case onto FALSE and we now have a boolean to return.

I'd say the first snippet is easier to follow, but it requires having a dummy role that is definitely expired.

NB: If you care about clean functions, sticking negatives in boolean method names is not a good idea for obvious reasons. Instead of Unexpired, perhaps isLive() or isValid() is a better idea.