CODEWITHSUNDEEP
javaexceptionjava-8option-type
Taobitz
Taobitz

Reputation: 556

Using Java 8 Optional - Converting logic

I'm trying to learn how to use Optional in Java - but this doesn't seem correct what I'm doing.

User user = null;

public AuthCheck(User user) throws Exception {
    if (user == null) {
        throw new Exception("No user!");
    }
    if (!(user.getStuff() != null && !user.getStuff().isEmpty())) {
        throw new Exception("User has no stuff");
    }
    this.user = user;
}

This is how I tried to convert it using Optional:

Optional<UserOptional> user;

public AuthCheckOptional(Optional<UserOptional> user) throws Exception {
    user.orElseThrow(() -> new Exception("No User!"));

    user.map(UserOptional::getStuff)
        .orElseThrow(() -> new Exception("User has no stuff"));

    this.user = user;
}

I would of thought I wouldn't need two separate checks. Also I believe I've changed the logic here as the IsEmpty() isn't happening.

Upvotes: 1

Views: 297

Answers (4)

akfp
akfp

Reputation: 57

I'd suggest a few restructuring to use Optionals more effectively.

  1. You should avoid calling the AuthCheck method with a null user in the first place, but you should have an Optional if it is null somewhere else
Optional<User> user = someWayToMaybeGetAUser();
user.map(AuthCheck::new)
  1. AuthCheck should probably just take in a User and a Stuff (and save both as independent fields).
public AuthCheck(User user, Stuff stuff) {
   this.user = user;
   this.stuff = stuff;
}

Then apply rule 1 again, and just don't pass in a null user and stuff to the the authcheck.

Optional<User> user;
Optional<Stuff> stuff = user.flatMap(User::getStuff);
Optional<AuthCheck> = user.flatMap(u -> stuff.map(s -> new AuthCheck(u, s));
// this is where using a more functional java library can help because you can use helper functions instead of the nested flatmaps

// Ex from cyclops
Option<AuthCheck> = user.forEach2(u -> stuff, AuthCheck::new); 
// Ex from vavr
For(user, stuff).yield(AuthCheck::new);

so then we can update the constructor to

public AuthCheck(User user, Stuff stuff) {
   this.user = Objects.requireNonNull(user);
   this.stuff = Objects.requireNonNull(stuff);
}

this keeps our data flow outside of the object, but still prevents incorrect authchecks from being created. Now, if you end up with a null being passed in, it's a programmer error.

Finally, the client code has a choice of what to do with an Optional authcheck if it doesn't exist, and if you want to throw specific exceptions arbitrarily you can add them wherever you want in the chain. Notice how forcing the data dependency avoids the problem with the other answer where chaining user into stuff loses the reason that the optional is empty (you can't decide at the end of the chain if user or stuff was the reason it was null).

User user = getMaybeUser().orElseThrow(() -> new Exception("No user"));
Stuff stuff = user.getStuff().orElseThrow(..."No stuff");
AuthCheck ac = new AuthCheck(u, s);

There's a bunch of ways to structure this, so it's more up to you.

Upvotes: 0

Andrew
Andrew

Reputation: 49606

I would like to warn you that it's insane and so confusing. And I see no reason to re-write the first good-looking approach*.

But if you really want to play with Optional, here's a method chain**:

Optional.of(
        Optional.ofNullable(user)
                .orElseThrow(() -> new Exception("No user!"))
)
        .map(User::getStuff)
        .filter(s -> !s.isEmpty())
        .orElseThrow(() -> new Exception("User has no stuff"));

*There is room for improvement as well. !(!a && !b) simply means a || b.

final String stuff = user.getStuff();
if (stuff == null || stuff.isEmpty()) {
    throw new Exception("User has no stuff");
}

**I assumed that getStuff returns a String.

Upvotes: 4

SeverityOne
SeverityOne

Reputation: 2691

You don't want to use Optional as an input parameter; it's an anti-pattern. Check out this article on DZone.

What you can do to improve your code is as follows: 

User user = null;

public authCheck(User user) {
    Objects.requireNonNull(user, "No user!");

    if (Objects.requireNonNull(user.getStuff(), "User has no stuff").isEmpty()) {
        throw new RuntimeException("User has no stuff");
    }

    this.user = user;
}

(Method names should start with a lower case in Java.)

You could further condense it, but the question is whether the code would be any clearer.

There's nothing wrong with null, if used properly. It means "undefined", and we don't have non-null object references in Java, like there are in Kotlin or Scala.

But perhaps you could think a little bit about how User objects are created, so that you avoid this issue altogether. You could perhaps employ the Builder design pattern. More often than not, rethinking your code can avoid situations like these.

Upvotes: 6

Ousmane D.
Ousmane D.

Reputation: 56423

You're using the Optional<T> type incorrectly and therefore you'll gain no benefit, rather it makes your code harder to understand and maintain.

You should also avoid passing Optional<T> as a method parameter, see here.

The non-optional approach is better for this type of logic and hence I'd stress that you proceed with that approach.

As @Cay S. Horstmann once mentioned in his book:

The key to using Optional effectively is to use a method that either consumes the correct value or produces an alternative.

Upvotes: 2

Related Questions