0

Is this good code style and good use of Option? This doesn't seem right because the method returns a Generics but the body does not make use of a Generics return. Also, 'smells' a bit bad because of the IF statement.

public Optional<MyObjectType> readData() {
    MyObjectType[] myArray = // Will not be null, but maybe Size 0.
    if(myArray.length > 0)
        return Optional.of(myArray[0]);

    return Optional.of(null);
}

On the client side I have: 

public void clientCode() {
    Optional<CurrencyPointData> oData = readCurrency(Currency.ADA);
    if(oData.isPresent()) {
        CurrencyPointData data = oData.get();
        // Use data object
    }
}

Which also doesn't seem to be much better than a normal if(oData == null) check.

Consequently, this code seems rubbish, so should I not be using Optional??

Adam Davies
  • 2,742
  • 4
  • 33
  • 52

3 Answers3

4

Your client code showcases the purpose of Optional.

It encourages code to check isPresent() before calling get(), highlighting the issue that there may be no value, unlike simply using CurrencyPointData without Optional, where it's then undefined whether return value can be null. Sure, you can document whether method can return null, but that's not apparent in the code, and we all know that people rarely reads the documentation in detail.

With Optional you explicitly say that value may be missing.

That is why Optional is better than simple null check.

Andreas
  • 154,647
  • 11
  • 152
  • 247
0

Consider not carrying Optional on the rest of your code with orElse.

Also, you can avoid 'if' with Option, it makes your code cleaner.

public void clientCode() {
    Optional<CurrencyPointData> oData = readCurrency(Currency.ADA);
    CurrencyPointData data = oData.orElse(new CurrencyPointData(...));

}
hagai
  • 424
  • 1
  • 7
  • 13
  • Interesting, but only useful if the client can work with a default value. If the producer can return a default value then it should and not use Optional. Nice take on the subject. – Adam Davies Feb 22 '18 at 21:22
0

Optional of null is bad. Use Optional.empty().

Although Optional forces you to handle null case before you can access the result, there is no point in using isPresent(). Using null would produce the same code but without needless Optional wrapper. Optional offers you a better alternatives to "unwrap" resulting value.

You could use orElse() for default value in case value is missing.

With orElseGet() you can calculate alternative value. This calculation is executed onlz in case of empty Optional.

Or use orElseThrow() to indicate that operation without value cannot continue.

There is also ifPresent() which is usefull when you have no use for the value apart of passing it to other object.

Januson
  • 4,533
  • 34
  • 42
  • No *point* in using `isPresent()`??? Then why did they add the method, if it's pointless? The method doesn't know how it'll be used, and because it can return without a value, it uses `Optional`. Sure, the call has nice methods on `Optional` to use, so code often won't have to use `if` statements, but there are definite cases where `isPresent()` is the correct way to write the code. It is certainly not a *pointless* method. – Andreas Feb 22 '18 at 19:20
  • Yes. Why would you wrap it in Optional just to check it with an if anyway? With null you get the exact same code just without The Optional. And although the isPresent might be rarely reasonable to use. There are better alternatives which should be prefered in most cases. – Januson Feb 22 '18 at 19:27
  • You wrap it in a `Optional` to document that value may be missing and forcing the caller to check for that. The method is `public` and can be called by anybody, meaning that it doesn't know that caller needs an `if` statement, so that cannot be part of the decision of whether method should return `Optional`. Your argument only makes sense if the method was `private`, so all callers are well-known, and you're in control if them all and can ensure they do correct null-checks. With `public` method you can't, and therefore you use `Optional`. That is *why* `Optional` exists, not the helpers. – Andreas Feb 22 '18 at 19:33
  • I think this is a strawman. I am not arguing that you should not use Optional. I am arguing that isPresent is the least-favorable of your Optional-unwrapping options. – Januson Feb 22 '18 at 19:40
  • Great response. I like this answer because of the discussion it created. I think the use of Optional to indicate optional return wins for me. Although I do feel Januson's frustration with the verbosness of Java, but I think that's something us Java heads have to get used to (unfortunately). – Adam Davies Feb 22 '18 at 21:25