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.
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.
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.
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.
@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.
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.
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.
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()
.
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.
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.
01 private void someMethod(Widget widget) {
02 // ... (More code)
Once again, passing a possibly-null value will generate a compile-time error.
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.)
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>
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 likeoptA.combine(optB, (a, b) -> doStuffWithNonNull(a, b))
or<Some Optional Class>.combine(optA, optB, optC, (a, b, c) -> dostuff(a, b, c))
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 whereTry<X, T extends Throwable>
andEither<L, R>
are available wrapper classes. It's like Optional, but without throwing away error information.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.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.And we can avoid an extra wrap if
this.userTableDao.findByUuid
can return Optional instead of just UserTable (I know hibernate can now returnOptional<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.