How to avoid checking for null values in method chaining?

Question

I need to check if some value is null or not. And if its not null then just set some variable to true. There is no else statement here. I got too many condition checks like this.

Is there any way to handle this null checks without checking all method return values?

if(country != null && country.getCity() != null && country.getCity().getSchool() != null && country.getCity().getSchool().getStudent() != null .....) {
    isValid = true;
}

I thought directly checking variable and ignore NullpointerException. Is this a good practice?

try{
    if(country.getCity().getSchool().getStudent().getInfo().... != null)
} catch(NullPointerException ex){
    //dont do anything.
}

well, you can end up having a `null` value on `getCity()`. this would cause an NPE and you wouldn't know for sure what is `null` — XtremeBaumer, Mar 20 '18 at 09:08
Just as a point of information: you should consider [Law of Demeter](https://en.wikipedia.org/wiki/Law_of_Demeter) before creating these kinds of chains. — Celos, Mar 20 '18 at 13:36
This code is a huge red flag: If you find yourself writing such deeply nested chains stop, reconsider, and rewrite your code. — Konrad Rudolph, Mar 20 '18 at 14:07
The best solution is to eliminate the problem: *implement those methods so that they never return null*. If they never return null then you don't have to check for it. Why is null a legal return value in the first place? — Eric Lippert, Mar 20 '18 at 21:10

khelwood · Accepted Answer · 2018-10-03T08:31:18.807

77

No, it is generally not good practice in Java to catch a NPE instead of null-checking your references.

You can use Optional for this kind of thing if you prefer:

if (Optional.ofNullable(country)
            .map(Country::getCity)
            .map(City::getSchool)
            .map(School::getStudent)
            .isPresent()) {
    isValid = true;
}

or simply

boolean isValid = Optional.ofNullable(country)
                          .map(Country::getCity)
                          .map(City::getSchool)
                          .map(School::getStudent)
                          .isPresent();

if that is all that isValid is supposed to be checking.

edited Oct 03 '18 at 08:31

answered Mar 20 '18 at 09:09

khelwood

46,621
12
59
83

10

Note that your two examples are only equivalent if `isValid` is `false` before your code is invoked! – Heinzi Mar 20 '18 at 12:20
1

"No, it is not good practice in Java to allow NPEs to happen." I disagree. If the data passed in is invalid, an exception should be thrown. And if the invalidity is that something that shouldn't be null was null, a NPE is appropriate. – Reinstate Monica Mar 20 '18 at 12:59
How many temporary objects does this code create? How much slower is it than the lengthy manual checking? – pts Mar 20 '18 at 13:29
1

@pts "How many temporary objects does this code create?" 4 temporary objects. "How much slower is it?" Very unlikely to make a noticeable difference in a real program. – khelwood Mar 20 '18 at 13:47
3

@pts: This code is also very amenable to inlining and escape analysis, so if this becomes a hotspot, there's a good chance the JIT will be able to elide allocating those temporary objects anyway. – Daniel Pryden Mar 20 '18 at 13:54
@Solomonoff'sSecret If the data passed in is invalid, many argue that an `IllegalArgumentException` should be thrown, even if what makes the data invalid is it being `null`. Yet many others agree with you that it should be an NPE. See https://stackoverflow.com/q/3881. – Mar 20 '18 at 21:21
in your first example it should be ifPresenr not isPresent, ifPresent takes a consumer, ifPresent returns a boolean – Ofer Skulsky Sep 07 '20 at 13:19
@OferSkulsky `ifPresent` is another possible method to use, but the code in my answer is correctly using `isPresent`. Using `ifPresent` with a Consumer would not allow you to reassign a local variable. – khelwood Sep 07 '20 at 13:21

score 16 · Answer 2 · answered Mar 20 '18 at 09:10

You could use Optional here, but it creates one Optional object at each step.

boolean isValid = Optional.ofNullable(country)
    .map(country -> country.getCity()) //Or use method reference Country::getCity
    .map(city -> city.getSchool())
    .map(school -> school.getStudent())
    .map(student -> true)
    .orElse(false);

//OR
boolean isValid = Optional.ofNullable(country)
                      .map(..)
                      ....
                      .isPresent();

@TejasKale Take a look at https://docs.oracle.com/javase/tutorial/java/javaOO/lambdaexpressions.html — user7, Mar 20 '18 at 17:42

score 7 · Answer 3 · answered Mar 20 '18 at 12:28

The object-oriented approach is to put the isValid method in Country and the other classes. It does not reduce the amount of null checks, but each method only has one and you don't repeat them.

public boolean isValid() {
  return city != null && city.isValid();
}

This has the assumption that validation is the same everywhere your Country is used, but typically that is the case. If not, the method should be named hasStudent(), but this is less general and you run the risk of duplicating the whole School interface in Country. For example, in another place you may need hasTeacher() or hasCourse().

Another approach is to use null objects:

public class Country {
  public static final Country NO_COUNTRY = new Country();

  private City city = City.NO_CITY;

  // etc.
}

I'm not sure it is preferable is this case (strictly you would need a sub class to override all modification methods), the Java 8 way would be to go with Optional as method in the other answers, but I would suggest to embrace it more fully:

private Optional<City> city = Optional.ofNullable(city);

public Optional<City> getCity() {
   return city;
}

Both for null objects and Nullable only work if you always use them instead of null (notice the field initialization), otherwise you still need the null checks. So this option avoid null, but you code becomes more verbose to reduced null checks in other places.

Of course, the correct design may be to use Collections where possible (instead of Optional). A Country has a set of City, City has a set of Schools, which has set of students, etc.

davidxxx · Answer 4 · 2018-03-21T04:46:04.580

As alternative to other fine usage of Optional, we could also use a utility method with a Supplier<Object> var-args as parameter.
It makes sense as we don't have many nested levels in the object to check but many fields to check.
Besides, it may easily be modified to log/handle something as a null is detected.

    boolean isValid = isValid(() -> address, // first level
                              () -> address.getCity(),   // second level
                              () -> address.getCountry(),// second level
                              () -> address.getStreet(), // second level
                              () -> address.getZip(),    // second level
                              () -> address.getCountry() // third level
                                           .getISO()


@SafeVarargs
public static boolean isValid(Supplier<Object>... suppliers) {
    for (Supplier<Object> supplier : suppliers) {
        if (Objects.isNull(supplier.get())) {
            // log, handle specific thing if required
            return false;
        }
    }
    return true;
}

Suppose you would like to add some traces, you could so write :

boolean isValid = isValid(  Arrays.asList("address", "city", "country",
                                          "street", "zip", "Country ISO"),
                            () -> address, // first level
                            () -> address.getCity(),   // second level
                            () -> address.getCountry(),// second level
                            () -> address.getStreet(), // second level
                            () -> address.getZip(),    // second level
                            () -> address.getCountry() // third level
                                         .getISO()
                         );


@SafeVarargs
public static boolean isValid(List<String> fieldNames, Supplier<Object>... suppliers) {
    if (fieldNames.size() != suppliers.length){
         throw new IllegalArgumentException("...");
    }
    for (int i = 0; i < suppliers.length; i++) {
        if (Objects.isNull(suppliers.get(i).get())) {
            LOGGER.info( fieldNames.get(i) + " is null");
            return false;
        }
    }
    return true;
}

This doesn't even work - you're supposed to chain functions (i.e. call with return value of previous function), not just call some functions in order. — SoniEx2, Mar 20 '18 at 23:00
I don't think so. Chained suppliers such as `address.getCountry,...,()-> address.getCountry().getISO())` tests first `getCountry()` and if not `null` test the `getISO()` method of the returned country. — davidxxx, Mar 21 '18 at 04:48
Calling `a.b()` then `a.b().c()` is not the same as `X tmp = a.b();` followed by `tmp.c()`. What if `b()` returns a different value every time you call it? Just because it's called `getXXX` doesn't mean it can't have side effects! — SoniEx2, Mar 21 '18 at 23:09
A getter with a side effect as invoked more once is a very bad practice and please read carefully the op question :"Is there any way to handle this null checks without checking all method return values?" .My answer addresses it. — davidxxx, Mar 22 '18 at 05:08

score 4 · Answer 5 · answered Mar 20 '18 at 09:33

Java doesn't have "null-safe" operations, like, for example Kotlin's null safety

You can either:

catch the NPE and ignore it
check all the references manually
use Optional as per the other answers
use some sort of tooling like XLST

Otherwise if you have control over the domain objects, you can redesign your classes so that the information you need is available from the top level object (make Country class do all the null checking...)

score 0 · Answer 6 · answered Mar 20 '18 at 14:15

0

You could also look at vavr's Option which as the below posts describes is better than Java's Optional and has a much richer API.

https://softwaremill.com/do-we-have-better-option-here/

answered Mar 20 '18 at 14:15

Gulats

432
5
11

The vavr Option class will not work as smoothly as the java Optional class in regards to the method chaining described in khelwood's accepted answer. With vavr, once the first Option test is non null, you receive a Some instance, as opposed to a None instance. Any subsequent calls to Some::map will return another Some instance, even if the map call returned a null value. This can lead to a null pointer if you chain another map call. To daisy chain with Option, you have intersperse the chain with non null check filters (filter(Optional::isPresent)).https://github.com/vavr-io/vavr/issues/2397 – JohnC Apr 16 '21 at 21:41

How to avoid checking for null values in method chaining?

6 Answers6

Linked

Related