6

Ok before I start explaining my question I want you to know that I know about the design idea behind Optional and that it isn't intended to be used in fields or collections, but I have programmed a lot in Kotlin currently and really dislike using null.

So I have a Node based editor like in the Unreal Engine and each node has ConnectionBoxes, which can be either free or are occupied by a Connection.

So there are different ways to express this one of which is using a map that maps each ConnectionBox to a Connection like:

Map<ConnectionBox, Connection> connectionEndPoints;

and Connection could be null if the ConnectionBox is free. I don't like this, since an other developer doesn't know about the function of this Map and that it may return null for an existing ConnectionBox.

So I am tempted to use a:

Map<ConnectionBox, Optional<Connection>> connectionEndPoints;

which displays the intend much better and you can even read it like:
"This ConnectionBox may have a Connection attached."

My question is: Why should I not do this, even though it shows the intend much more clearly and prevents NPEs. Every SO-thread and every blog post says this is bad style and even the compiler say that I shouldn't do it with a warning.


On request here is a SO-thread that discourages use of Optional as a field or Collection value: Uses for Optional

And here is the warning (as it turns out it is a warning from IntelliJ):
Warning: Optional used as type for field {fieldName}


Ok after recommending that the Connection reference should lie within the ConnectioBox the question just shifts.

Why is:

class ConnectionBox {
    Optional<Connection> connection;
    ...

worse than

class ConnectionBox {
    Connection connection; //may be null
    ...

Unless I make a comment you can't see that you may run into a NPE and I dislike comments explaining code that could explain itself.

Neuron
  • 5,141
  • 5
  • 38
  • 59
Jhonny007
  • 1,698
  • 1
  • 13
  • 33
  • 1
    Why isn't `Connection` a property of `ConnectionBox`, since the latter seems to be a container for the former? – Kayaman Oct 21 '16 at 12:07
  • Why didn't I think of that... probably need more coffee -.- But this still just shifts the question -> why is it recommended to not use `Optional` as a field in `Connection` because the `Optional` still displays the intend much better than having a nullable field (which you can't see when looking at the field). – Jhonny007 Oct 21 '16 at 12:16
  • 2
    Have you considered doing this? `map.getOrDefault(connectionBox, new EmptyConnection())` where empty connection is a null wrapper for connection? – Ash Oct 21 '16 at 12:19
  • 2
    Could you link some SO-thread or blog post which says this? The warning message itself would be also useful. – pcjuzer Oct 21 '16 at 12:21
  • 2
    @AshFrench I'd rather use [`computeIfAbsent`](https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function-) because `getOrDefault` creates an instance of `EmptyConnection` even if there's a connection. – beatngu13 Oct 21 '16 at 12:29
  • 2
    My problem more lies within the readability. When using `Optional` you immediately see that it is **optional**. I know that I can use `computeIfAbsent` to deal with `null` but nobody can see that there is the possibility of `null` when looking at the field. – Jhonny007 Oct 21 '16 at 12:36
  • You can initialize to your default value rather than being null, or using an Optional. – matt Oct 21 '16 at 13:44
  • You are strongly advised against using a mutable object as the key of a `HashMap`. Your map will become seriously damaged if the value of the key changes while it is contained by the map. – scottb Oct 21 '16 at 13:44
  • @scottb don't worry my `hashCode()` method is overridden and only uses a unique id which is immutable. – Jhonny007 Oct 21 '16 at 14:08
  • @Jhonny007 the link to the Unreal Engine node editor is broken. Can you find a replacement? I left the link at the bottom of your question as a comment – Neuron May 29 '18 at 08:01

1 Answers1

1

Edit

After watching Stuart Marks' (who works for the core libraries team in the JDK group at Oracle) talk "Optional – The Mother of All Bikesheds" from Devoxx 2016, you should jump to 54:04:

Why Not Use Optional in Fields?

  • More a style issue than a correctness issue
    • usually there's a better way to model absence of a value
    • use of Optional in fields often arises from slavish desire to eliminate nullable fields
    • remember, eliminating nulls isn't a goal of Optional
  • Using Optional in fields...
    • creates another object for every field
    • introduces a dependent load from memory on every field read
    • clutters up your code
    • to what benefit? ability to chain methods?

Original Post

According to IntelliJ's inspector (Preferences > Editor > Inspections > 'Optional' used as field or parameter type):

Optional was designed to provide a limited mechanism for library method return types where there needed to be a clear way to represent "no result". Using a field with type java.util.Optional is also problematic if the class needs to be Serializable, which java.util.Optional is not.

This also applies to collections in case you have to serialize them. Furthermore, have a look at these links:

  • Java 8 Optional: What's the Point?

    So to recap - in an attempt to get rid of NullPointerExceptions we have a new class that:

    • Throws NullPointerExceptions
    • Can itself be null, causing a NullPointerException
    • Increases heap size
    • Makes debugging more difficult
    • Makes serializing objects, say as an XML or JSON for an external client, much more difficult
  • Why java.util.Optional is broken

    The final irony is that by attempting to discourage nulls, the authors of this class have actually encouraged its use. I'm sure there are a few who will be tempted to simply return null from their functions in order to "avoid creating an unnecessary and expensive Optional reference", rather than using the correct types and combinators.

If you care about readability, you could also use @Nullable (available in Eclipse as well as in IntelliJ):

class ConnectionBox {
    @Nullable
    Connection connection;
    // ...
}

Alternatively, you can create an optional getter:

class ConnectionBox {
    Connection connection;
    // ...
    Optional<Connection> getConnection() {
        return Optional.ofNullable(connection);
    }
}
Community
  • 1
  • 1
beatngu13
  • 7,201
  • 6
  • 37
  • 66
  • I am not quite sure how to use these Annotations. IntelliJ only shows me `com.sun.istack.internal.Nullable` which obviously shouldn't be used as it lies within `sun`. And even if I use IntelliJ's Annotation and someone else uses Eclipse does the code still compile for them? Or for someone who uses Vim can they compile it? – Jhonny007 Oct 21 '16 at 13:53
  • @Jhonny007 annotations are just like libraries, you can simply import them. For instance, JUnit's infamous `@Test` can be used as soon as the dependency is within your classpath—no matter which IDE/editor. Hence, you can simply [add the JAR to your proect](https://www.jetbrains.com/help/idea/2016.2/enabling-annotations.html). Regarding Eclipse: have a look at [this](http://stackoverflow.com/q/20549153/3429133) post. – beatngu13 Oct 21 '16 at 14:46