Is it valid to add a Null check and access the Integer value in the same line of if condition in Java

Question

Say

class Person{
    Integer height;
    integer weight;
}

is it valid to check like this?

Person p = new Person();
if (p.height !=null && p.height >= 1 && p.weight >=1 ){}

Answer 1

It's fine, but note that this will still crash with a NullPointerException if p.weight is null. If you want clean code, consider this question:

What does height is null actually mean? Does it mean:

• It is semantically equivalent to 0. (Then, why have it? Make your fields int, and set them properly).
• It is unknown.
• It is unset; this is a person who does not want their height publicized.

In particular, given that you are checking for >= 1, apparently you can have a null height, but also a negative height. What is the semantic difference between these two? If there is no difference, why are you allowing a cavalcade of different internal values that nevertheless all boil down to representing the same state? You're signing up for a bevy of checks everytime you interact with these variables, and a combinatorial explosion to test all this. Don't do it this way - create a single value to represent 'invalid' or 'unknown' or 'intentionally omitted' or whatever it is that you need to convey.

Usually it leads to better code if you [A] eliminate invalid state as early as possible, which in particular means that you do not need to check for invalid state (here, 0 and negative numbers appear to be intended as invalid) every time you use these variables, and [B] use a sentinel value and not null to indicate a unique state such as 'unset' or 'intentionally not shared'.

In other words:

• Make height and weight private
• Their setters will refuse to set (and throw IllegalArgumentException instead) if trying to set 0 or negative height or weight.
• The fields are of type int
• Constants exist for the various alternate states.

public class Person {
    private static final int UNKNOWN = -1;
    private static final int INTENTIONALLY_OMITTED = -2;
    private int height, weight;

    public Person() {
      this.height = UNKNOWN;
      this.weight = UNKNOWN;
    }

    public void setHeight(int height) {
      if (height < 1) throw new IllegalArgumentException("Non-positive height");
      this.height = height;
    }

    public void setHeightOmitted() {
      this.height = INTENTIONALLY_OMITTED;
    }
}

and so on. Now you can write code that is inherently readable; null is nebulous (you'd have to document what it means. Does it mean unset, or invalid, or intentionally omitted? What?), if (height == INTENTIONALLY_OMITTED) documents itself, that's a good thing.