SwingGuy1024/BadOptional vs NullnessChecker.md

## BadOptional vs NullnessChecker.md

      
    Raw
  

              BadOptional vs NullnessChecker.md
            
          
    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.)