Skip to content

Instantly share code, notes, and snippets.

@SwingGuy1024
Last active July 10, 2019 03:20
Show Gist options
  • Save SwingGuy1024/cfe93744fee7ef0812611a89bc884ef5 to your computer and use it in GitHub Desktop.
Save SwingGuy1024/cfe93744fee7ef0812611a89bc884ef5 to your computer and use it in GitHub Desktop.
Misuse of Optional

Java Optional, Java's Nullness Flaw, and the Kotlin Language

This is a work-in-progress. There's a lot here that I plan to rewrite, but the opening section, including and ending with the five examples, is very close to its final form.

Many people, including me, were excited that an Optional class was introduced in Java 8. As soon as I saw it, I was disappointed by how verbose it was, and puzzled by the strange and long-winded approach. At the time, I misunderstood its purpose, as did a lot of other people. Many people began to use it extensively. It's now very widespread, and it has led to some amazingly bad code, written by people who don't understand what it's for. In certain limited situations it's actually very useful, although I rarely actually use myself. Here's my take on Java's Optional class. (Some of this is taken from a StackOverflow question that I answered. This expands on my answer.)

Optional's Purpose

Optional was introduced at the same time as functional programming. According to a blog post by Brian Goetz), it was added because the return expression in example 2 is cleaner than the three lines of code in example 1:

Example 1: Clumsy code

Method matching =
  Arrays.asList(enclosingInfo.getEnclosingClass().getDeclaredMethods())
  .stream()
  .filter(m -> Objects.equals(m.getName(), enclosingInfo.getName())
  .filter(m ->  Arrays.equals(m.getParameterTypes(), parameterClasses))
  .filter(m -> Objects.equals(m.getReturnType(), returnType))
  .getFirst();
if (matching == null)
  throw new InternalError("Enclosing method not found");
return matching;

Example 2: Cleaner code, in a single expression

return Arrays.asList(enclosingInfo.getEnclosingClass().getDeclaredMethods())
  .stream()
  .filter(m -> Objects.equals(m.getName(), enclosingInfo.getName())
  .filter(m ->  Arrays.equals(m.getParameterTypes(), parameterClasses))
  .filter(m -> Objects.equals(m.getReturnType(), returnType))
  .findFirst()
  .getOrThrow(() -> new InternalError("Enclosing method not found"));

My point is that Optional was written to support function chaining, which is crucial to the new functional programming paradigm. That's the only place where I use it. It's used here in the findFirst() method, which, after all the filtering, mignt not even have a value. The findFirst() method here replaces the getFirst() method from example 1, which the code has to test for null to guarantee that the return value is not null.

Another place where it gets used is in function parameters for the very useful Optional.flatMap() method, but not in the Optional.map() method. Here are their signatures:

public <U> Optional<U> flatMap(Function<? super T, ? extends Optional<? extends U>> mapper)
public <U> Optional<U> map(Function<? super T, ? extends U> mapper)

The two methods do the same thing, but flatMap() takes a Function that returns Optional, while map() takes one that doesn't. This is a useful way to write an API. The two methods do the same thing, but the user has a choice. So Optional will only be used when a method might actually return null.

But all-too-often it gets used where Required might be more descriptive. It's also used a lot of places where it really isn't very helpful, or it's so verbose that it's clumsy. Rather than add to the endless debate about how to use it, I'd like to illustrate just how badly it's getting used. Here are some examples, all taken from actual production code.

Bad Code Examples

Example 1: Needless Verbosity

01 private void someMethod(Widget widget, ... <other parameters>) {
02   Optional<Widget> widgetOpt = Optional.of(widget);
03   if (!widgetOpt.isPresent) {
04     throw new BusinessException("Missing Widget");
05   }
06   // ... widgetOpt is never used again
07   // ... several lines later
08   Optional<Widget> widgetMaybe = Optional.of(widget);
09   processData(Optional.of(widgetMaybe));
10   // ...more code
11 }

What's going on here? One lines 2 and 3, the coder is taking two lines to do a simple null check. Then it throws widgetOpt away, and it creates a second Optional of the same value on line 8.

The first use is needlessly verbose, and seems to be done just to avoid ever using the null keyword in the code, as if it's use is now taboo. Lines 02 - 05 could have been written like this:

02   if (widget == null) {
03     throw new BusinessException("Missing Widget");
04   }

This is cleaner, more readable, and even faster.

But the second use of Optional is necessary. But it's necessary not because they're doing functional programming, but merely because someone wrote a method that takes an Optional<Widget> as a parameter.

To make matters worse, there's a repeated bug in lines 02 and 08 that didn't get caught. It says Optional.of(widget), but it should say Optional.ofNullable(widget)! So if a null value ever gets passed into this method, it will throw a NullPointerException instead of the BusinessException thrown on line 04. But nobody caught this error for two reasons. First, the unit test only tested for valid input. A proper unit test will also test for the proper exceptions getting thrown when invalid data is passed in. Second, The code that called this private method had already made sure that widget was not null, so the test was unnecessary.

Example 2: Misleading Return values

Here's a class that was generated by Swagger 2.0, using their Swing server generator, with the Java 1.8 option turned on:

@Controller
public class MenuItemApiController implements MenuItemApi {
    private final ObjectMapper objectMapper;
    private final HttpServletRequest request;

    @Autowired
    public MenuItemApiController(ObjectMapper objectMapper, HttpServletRequest request) {
        this.objectMapper = objectMapper;
        this.request = request;
    }

    @Override
    public Optional<ObjectMapper> getObjectMapper() {
        return Optional.ofNullable(objectMapper);
    }

    @Override
    public Optional<HttpServletRequest> getRequest() {
        return Optional.ofNullable(request);
    }
}

This has two private final members, and two getters that wrap the values in an Optional. But take a look at that constructor. It's annotated with @Autowired. Spring will automatically instantiate valid values for both of the parameters and inject them into the constructor. And they're final, so they'll never change to null. The methods are returning objects that can never be null, wrapped inside an Optional. Does the use of Optional here add clarity to the API? It actually does the opposite: It suggests that two never-null objects might actually be null, encouraging users to write code to check for null values that they'll never see.

Example 3: Misleading Parameters

Many people have written guidelines recommending against using Optional as parameter methods, but people do it anyway. A common variation of the code in example 1 looks like this:

01 private void someMethod(Optional<Widget> widgetOpt) {
02   if (!widgetOpt.isPresent) {
03     throw new BusinessException("Missing Widget");
04   }
05   // ... (More code)

Again I have to ask: What clarity is being added to the API by using Optional? To anyone reading the JavaDocs, the API implies that null is a valid input value. But once you look at the code, it's clearly not. A case could be made for using Optional when null is actually allowed, but so many methods are written like this one that the point will get lost. (Not that it's a good idea anyway.) But this case, where an Optional is required where a null value throws an exception, results in a misleading API that's more verbose to call, since the user must now write someMethod(Optional.ofNullable(widget)); instead of someMethod(widget); A method signature should take care of boilerplate details needed to call the method, to make the call as simple as possible. By using Optional in a parameter, this does the opposite. This method uses Optional to mean Required.

Example 4: Seemingly Sensible Use

Here's a case where it may actually make sense to use Optional on a method parameter:

01 private void someMethod(Optional<Widget> widgetOpt) {
02   final Widget widget = widgetOpt.orElse(getDefaultWidget());
03   // ... (More code)

Here, finally, we can say that Optional is adding clarity to the API. Use of null instead of a Widget instance is actually allowed. Here, the API does not mislead anyone. Of course, users who choose to use null must be careful not to call Optional.of(widget). Instead, they should use Optional.ofNullable(widget) or Optional.empty(), but that's a fail-fast mistake, so it will get caught early. Unfortunately, so many developers wrap required parameters inside Optional, that the meaning of this occasional valid use will often get lost anyway. And, there's a simpler way to write the API that doesn't add verbosity to the calling method:

01 public void someMethod(final Widget widgetOrNull) {
02   final Widget widget = widgetOrNull == null? getDefaultWidget() : widgetOrNull;
03   // ... (More code)

That's right. Simply renaming the parameter will provide the same information as Optional. Before Optional was invented, not many people did this, which is probably a shame, because it adds clarity without adding verbosity.

someMethod(Optional.ofNullable(widget));  // The first method should be called like this.
someMethod(widget);                       // The second may be called like this.

Furthermore, Java already has a way to let the user know a parameter is optional, and has had it since Java 1.0. It's called overloading. This could have been written as two methods:

01 public void someMethod() {
02   someMethod(getDefaultWidget());
03 }
04
05 public void someMethod(Widget widget) {
06    // ... (More code)
07 }

Example 5: Ridiculous

01 private void someMethod(Optional<Widget> widgetOpt) {
02   if (!widgetOpt.isPresent) {
03     throw new NullPointerException();
04   }
05   Widget widget = widgetOpt.get();
06   Widget.someMethod();             // Without Optional, the NullPointerException will get thrown here.
07   // ... (More code)

Yeah. I've seen this.

Why All the Affection for Optional?

Why would anyone write code like this? Probably because Optional is being used to address a very real flaw in the language design. I call this the Nullness Flaw, and it's responsible for much of the misuse of Java's Optional class.

The Nullness Flaw

The Nullness Flaw is this: There's no way to specify which of an API's parameters and return values are allowed to be null. It may be mentioned in the javadocs, but most developers don't even write javadocs for their code, and not many will check the javadocs as they write. So before Optional was added to the language, a lot of people wrote code like this:

void someMethod(Widget widget, List<Integer> data) {
  if (widget != null) {
    process(widget, data);
  }
}

private void process(Widget widget, List<Integer> data) {
  if (widget != null) {     // Widget was already tested for null in the previous method!
    ...
  }
}

On a large project of a past employer, this was done all over our code, and the ironic thing was that widget couldn't possibly be null here, because it had already been validated the same way by the method that calls someMethod(), where it also couldn't be null for the same reason. Once I checked a value in a debugger and found out that it has been repeatedly checked for null nine times up the call stack before ever reaching the method I was debugging.

This is a very bad practice, because it hides bugs. If widget is null here, the method will still fail, but no exception will be thrown. In fact, the stated purpose of this practice was to prevent exceptions from ever getting thrown in production code, as if this prevents bugs. (It doesn't.) Java's Exception classes were written to include a detailed stack trace even when not running in a debugger, which was one of many improvements over C++, but this coding practice throws all this useful information away, just to create the illusion that their code doesn't have bugs. And the customer aren't fooled. They notice when an operation fails to do anything.

I think there was a real thirst to solve this flaw in Java, because so many people saw the new Optional class as a way to add clarity to APIs. This is a laudable goal, but it's not why Optional was created. And it's a bad way to accomplish this. And the way Optional is commonly used, it actually makes the APIs less clear. By using Optional in parameters where it's clearly not needed, many developers are just writing the same bad code using a more verbose idiom.

Better Solutions

IntelliJ Annotations

I have been using a very good solution to this API flaw for several years now, but it's only available to users of the IntelliJ IDE. Intellij provides annotations @Nullable and @NotNull, which are used with a code inspection that flags variables that violate the annotations' restrictions. So the method above could be written like this:

void someMethod(@NotNull Widget widget, List<Integer> data) {
    process(widget, data);
}

The IDE will then check the code that calls this method to make sure the value can't be null, and generates an inspection failure if it gets called incorrectly. Unlike the Optional class, this changes a null pointer exception from a runtime exception to a compile time error. IntelliJ also includes a @Contract annotation that lets you specify under what conditions a method may return null. I have found this to be a very powerful solution to the problem, but the chief drawback to this is that it only works when using the Intellij IDE. When I'm on a team that has standardized on Eclipse, Netbeans, or some other IDE, I'm often the only one who can see the inspection failures.

I've gotten strange pushback against this approach. Most commonly I get the claim that we don't want to be dependent on an external tool. I find this a strange argument, since the annotations provide an additional level of checking for our code. So for Eclipse users, there's may be no advantage to using these annotations, but there's no drawback either. Annotations were designed with the understanding that the compiler will ignore any annotations that it doesn't understand, to prevent their use from creating a dependency.

Fortunately, IntelliJ can be configured to use other annotations for the same purpose, so you can set it to use, say, the findBugs annotations, or you can add custom annotations to your project and use those instead.

IntelliJ has a way to declare the annotations externally, so users of Eclipse or Netbeans won't object to them because they never even see them. It's a bit of a pain to set up, but it lets you work around the objections of others, especially when project management has forbidden you to put the annotations in the code. (Yes, this has happened to me.)

The strangest pushback I've seen is this: I've been told that this can't possibly work, because Alan Turing proved that it's impossible to determine if a program is correct. Except that's not what Alan Turning proved. He proved that for any algorithm designed to prove a progam will complete successfully, it is possible to write some code that will defeat your algorithm. When applied to the IntelliJ annotations, this proof simply states that it's possible to write an algorithm that may still have a bug even if the inspection passes. This doesn't mean the inspections will never work, just that they won't catch all possible bugs, which I don't expect them to do anyway. And IntelliJ's null code inspection isn't even trying to prove that the program is correct, it's just trying to catch a certian specific bug.

The Nullness Checker

The Checker Framework provides a Nullness checker that works as an annotation processor, which means it can be used by any IDE, and hence isn't subject to some of the objections raised by the IntelliJ annotations. The Nullness checker (one of several suppported checkers) also uses annotations, called @NonNull and @Nullable. It doesn't work as well as the IntelliJ system because it doesn't include anything to do what @Contract does in IntelliJ. It provides additional annotations for use during object construction, where variables can be temporarily null, but those annotations are hard to use correctly, and the documentation doesn't always have useful examples. But it's more universal than IntelliJ.

I've only used it with new projects. It probably produces a lot of warnings in existing projects written with lots of superfluous tests for null, so it might be tough to retrofit.

Kotlin

The cleanest solution may well be to switch to the Kotlin language. Kotlin compiles to Java byte code, so it integrates well with your Java code. But unlike Java, it lets you specify exactly where null values are allowed, and where they're forbidden. In Kotlin, a variable of type Widget can be declared one of three ways:

var widgetOne: Widget?
var widgetTwo: Widget = new Widget()
var widgetThree = new Widget()

Here, the question mark tells the compiler that widgetOne may be null. But widgetTwo and widgetThree are declared to never be null. Also, widgetThree's declaration uses an inferred type — the compiler figures out that the type is a Widget from the constructor call. Now, if the method from the previous example is declared like this:

fun someMethod(widget: Widget, data: List<Int>) {
  process(widget, data);
}

The compiler will know that Widget is not allowed to be null, so if I were to pass widgetOne to this method without first setting a value, it will generate a compiler error.

Of course, if I have some method where I want to allow a parameter to be null, I can write it like this:

fun someMethodWithOptional(widget: Widget?, data: List<Int>) {
  ...
}

Kotlin is smart enough to figure out when an optional object is no longer null, so I can say this:

fun someMethod(widget: Widget? data: List<Int>) {
  if (widget != null) {
    someMethod(widget.getName(), data)
  } else {
    ...
  }
}

Kotlin knows that widget is no longer null inside the if branch, so it effectively changes the type from Widget? to just plain Widget.

(Kotlin has many other nice features that are beyond the scope of this essay.)

Conclusion

When facing the Nullness flaw in the Java language, the IntelliJ annotation, the Nullness Checker, and the Kotlin language are all much better solutions than the Optional class. Optional was written for use in Functional programming, and that's the only place I use it. In fact, the java.util.function package is the only one where in the Java API where Optional is used.

My rules for Optional

  1. Never use it as a method parameter.
  2. Never use it as a member of a class.
  3. Never use it in a constructor.
  4. Only use it as a return value if the value could actually be null.
  5. Wherever possible, I design my classes so that members can't possibly be null once the object is constructed. If necessary, I write a Builder class to do the actual construction. (See Effecive Java by Joshua Bloch for a good discussion of Builders.)

I always use one of the three approaches to working around the Nullness flaw. It's a game changer. I love it. My code is much cleaner and more reliable because of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment