Let's say we have a class Product which has some nullable fields that are allowed to be null, for example, quantityAvailable and quantityReserved.

I may want to validate these fields and throw an error, but since they are allowed to be null, I have, firstly, to check for null:

if (product.getQuantityAvailable() != null && product.getQuantityReserved() != null) {
  if (product.getQuantityAvailable() < product.getQuantityReserved()) {
    System.out.println("Error");
  }
}

When you have many nullable fields and many null-checks, this becomes tedious. Even with ofNullable it doesn't get any better.

Optional.ofNullable(product.getQuantityAvailable())
    .ifPresent((available) -> {
          Optional.ofNullable(product.getQuantityReserved())
              .ifPresent((reserved) -> {
                    if (available < reserved) {
                      System.out.println("Error");
                    }
                  });
        });

Ideally there would exist some annotation that would check for nulls and skip if any were found, but obviously this wouldn't work.

if (@Nullable product.getQuantityAvailable() < @Nullable product.getQuantityReserved()) {
  System.out.println("Error");
}

Is there a solution to avoid the boilerplate above?

CodePudding user response：

I think you should define models with default values for additional attributes instead of accept null value. Example, with an Integer attribute, default value is 0 and String attribute is empty string

CodePudding user response：

I would not discount Optional, OptionalInt and such, even if the persistent database column uses NULL.

Indeed it would be cumbersome:

product.getQuantityAvailable()
    .ifPresent((available) -> {
          product.getQuantityReserved()
              .ifPresent((reserved) -> {
                    if (available < reserved) {
                      System.out.println("Error");
                    }
                  });
        });

But with a map from OptionalInt to IntStream:

if (product.getQuantityAvailable()
        .map().anyMatch(av -> product.getQuantityReserved()
            .map.anyMatch(res -> av < res))) {
    System.out.println("Error");
}

You could store the predicate lambdas separately, say in the product.

The code is more readable, less errorprone, though a bit )))-ish.

CodePudding user response：

Firstly, there's nothing wrong with explicit null-checks.

Secondly, Optional.ofNullable() is not meant for validation. By hiding a null-check with it, you're obscuring what the code does and making it harder to read.

Here's a quote from the answer by @StuartMarks, Java and OpenJDK developer, regarding what Optional is meant to be used for:

The primary use of Optional is as follows:

Optional is intended to provide a limited mechanism for library method return types where there is a clear need to represent "no result," and where using null for that is overwhelmingly likely to cause errors.

A typical code smell is, instead of the code using method chaining to handle an Optional returned from some method, it creates an Optional from something that's nullable, in order to chain methods and avoid conditionals.

Also have a look at this answer by Stuart Marks "Should Optional.ofNullable() be used for null check?".

Using Optional.ofNullable() to avoid null-checks is an antipattern. You shouldn't create Optional in order to validate an object that can be null

I see nothing that can be considered to be "bad" in this snippet from your question. Yes, it's verbose, it's a price for allowing nullable fields.

if (product.getQuantityAvailable() != null && product.getQuantityReserved() != null) {
    if (product.getQuantityAvailable() < product.getQuantityReserved()) {
        System.out.println("Error");
    }
}

If you want to reduce the level of nesting, here's a null-safe alternative with Objects.compare():

if (Objects.compare(product.getQuantityAvailable(), 
                    product.getQuantityReserved(), 
                    Comparator.nullsFirst(Comparator.naturalOrder())) < 0) {
    // perform the required action
    System.out.println("Error");
}

If you want to reduce the number of null-checks in your code, then assign default values (0 for Integer,BigDecimal.ZERO, empty collections, etc.) instead of keeping null references in every place where it doesn't harm the logic of the application.