Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Examples of bad usages of Optional that I've seen in production code.

Bad Use of Optional: Code Examples

I was disappointed by the introduction of Optional to the Java language, because I've been using @Nullable and @NotNull annotations for years, and it's a much simpler solution. The big advantage of this approach is that it makes null pointers a compile-time error rather than a run-time bug. Now that I've worked with Optional and seen it in action in production code, I've only strengthened my earlier view. Here are some examples, all taken from production code, of bad usages of Optional, with notes on what it would look like with the NullnessChecker annotation processor instead.

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); // this duplicates the previous Optional!
09   processData(widgetMaybe);
10   // ...more code
11 }

What's going on here? On lines 2 and 3, the coder is taking two lines to do a simple null check. Then widgetOpt is thrown away, and a second Optional of the same value is created on line 8, because they call a method that requires it.

The first use is needlessly verbose, and seems to be done just to avoid ever using the null keyword in the code, as if bansishing it will prevent null-pointer bugs. (It won't.) Lines 02 and 03 could have been made cleaner, more readable, and even marginally faster by writing if (widget == null) {

The second use of Optional is necessary only because the API they're using requires it.

To make matters worse, there's a bug in line 02 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. (The second use of Optional.of() in line 08 is fine, because we already know it's not null.) But nobody caught this error for two reasons. First, There was no unit test. Second, The code that called this private method had already made sure that widget was not null, so the test was unnecessary.

With the Nullness Checker

01 private void someMethod(Widget widget, ... <other parameters>) {
02   // ... several lines later
03   processData(widget);
04   // ...more code
05 }

The annotation processor defaults to @NonNull, so if I added a null-check, it would tell me that it's unnecessary. The annotation processor knows that the calling method already checks for null, so it's happy with this. If somebody later removes the test for null in the calling method, it will generate a compile-time error where this method is called.

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, forcing users to write extra code to check for null values that they'll never see.

With the Nullness Checker

@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 ObjectMapper getObjectMapper() { return objectMapper; }

    @Override
    public HttpServletRequest getRequest() { return request; }
}

Again, this is simpler, because @NonNull is assumed. The constructor can't get called with null values, so we don't bother with Optional.

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 looking at the API, it implies that null is a valid input value. But once you look at the code, it's clearly not. Putting Optional on a parameter that shouldn't be null results in a misleading API that's also more verbose to call, since the user must now write someMethod(Optional.ofNullable(widget)); instead of someMethod(widget); A good interface should take care of boilerplate details needed to perform its function. By using Optional in a parameter, this forces the user to add a bit of boilerplate that doesn't add any value.

With the Nullness Checker

01 private void someMethod(Widget widget) {
02   // ... (More code)

Again, @NonNull is assumed, so we don't need to check for null. A value that may be null will generate a compiler error.

Example 4: Seemingly Sensible Use

Here's a case where putting Optional on a method parameter doesn't mislead the user:

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 probably get caught early. Unfortunately, so many developers wrap mandatory 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 private void someMethod(final Widget widgetOrNull) {
02   final Widget widget = widgetOrNull == null? getDefaultWidget() : widgetOrNull;
03   // ... (More code)

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. It also eliminates the (minor) bug that will arise if the calling method uses Optional.of() instead of Optional.ofNullabe().

With the Nullness Checker

01 private void someMethod(@Nullable widget) {
02   widget = (widget == null) ? getDefaultValue() : widget;
03   // ... (More code)

The @Nullable annotation becomes part of the API, so users can see that a null value is allowed. The Nullness checker will figure out that widget is not null, and proceed from that assumption.

Example 5: Pointless

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

Yeah. I've seen this. Just get rid of lines 2 through 4. It does exactly the same thing.

With the Nullness Checker

01 private void someMethod(Widget widget) {
02   // ... (More code)

Once again, passing a possibly-null value will generate a compile-time error.

Example 6: OrElse(null)

01    public User findByUuid(@NotNull String userUuid) {
02        UserTable userTable = (UserTable)this.userTableDao.findByUuid(userUuid).orElse(null);
03        return userTable != null ? this.userServiceConverter.convertFromUserTable(userTable) : null;
04    }

Huh? (More on this later.)

@ayush-finix

This comment has been minimized.

Copy link

ayush-finix commented Apr 5, 2019

Cool set of real cases. My only concern is that it seems like most of these cases aren't very sensible in general. If you're never calling optional.map or flatmap, then the code is just going to be terribly verbose procedural code. I know you didn't ask for it, but here are my thoughts on the examples. As purely an aside, I also hate void methods, so I'm not a big fan of code using Consumer<X>

  1. First, this seems like it should be a method on the widget class itself, but if it can't be, it seems like the calling code should instead do optWidget.ifPresent(w -> processData(w));. If you need to combine multiple optionals together then do something, you're probably going to have to use a better functional library for java, where there's an actual optional.join/combine/zip type of method. There's some nice libraries where you can do things like optA.combine(optB, (a, b) -> doStuffWithNonNull(a, b)) or <Some Optional Class>.combine(optA, optB, optC, (a, b, c) -> dostuff(a, b, c))
  2. I've written tons of spring apps, and I don't think I've needed to actually have getters for injected components. I feel like having getters for those is a code smell in general, but it's generated code, so w/e.
  3. Agreed this is terrible, but not for the same reason. The calling code should clearly be doing some variation of widget.map(w -> someMethod(w)).orElseThrow(() -> new BusinessException()) Of course, if widget is coming from like the top of the call stack, this seems like a case where null check and throw as validation seems correct. For what it's worth, I don't throw exceptions, but use a library where Try<X, T extends Throwable> and Either<L, R> are available wrapper classes. It's like Optional, but without throwing away error information.
  4. An even better way to write that api is to write a method that does not accept null, and have the calling code figure it out with the map function. Having to write every function (of a non library codebase) super defensively is already a code smell. Figure out where you're validating your parameters, and the layers after that shouldn't need to handle nulls.
  5. Same problem as 4. Like why isn't this just optWidget.ifPresent(w -> func(w)) on the calling side. If you need to know if something actually happened, I'd suggest not having so many void methods and using useful return values.
  6. This method should clearly return an Optional and should be written
public Optional<User> findByUuid(@NotNull String userUuid) {
    return Optional.ofNullable(((UserTable)this.userTableDao.findByUuid(userUuid)))
               .map(ut -> this.userServiceConverter.convertFromUserTable(ut))
}

Could probably even get a method reference to do .map(userServiceConverter::convertFromUserTable) instead. If you have to keep the signature the same, the .orElse(null) can come at the very end.

public User findByUuid(@NotNull String userUuid) {
    return Optional.ofNullable(((UserTable)this.userTableDao.findByUuid(userUuid)))
               .map(ut -> this.userServiceConverter.convertFromUserTable(ut))
               .orElse(null)
}

And we can avoid an extra wrap if this.userTableDao.findByUuid can return Optional instead of just UserTable (I know hibernate can now return Optional<T> instead of T for it's find methods, but idk where your daos come from (I use jooQ)). As a side question, why would the client of something called userTableDao need to cast the response of a find method to a UserTable??? Like, how is not like usertabledao implements basedao<UserTable, UUID> or something?

TLDR: optional is not supposed to replace null checks, it's supposed to remove them. If you replace every == null with Optional.isPresents, you're adding a bunch of cost and missing like all of the benefits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.