If final object is being passed, should null still be checked?

Question

Check out the following code:

    @Override
    public int compareTo(final Intersection o) {
        if (o == null) 
            return 0;

        double distance = t;
        double distance2 = o.t;

        if (distance == distance2) 
            return 0;


        return distance2 > distance ? -1 : 1;
    }

All seems well however the fact that 0 may be returned on two different conditional occasions does somewhat bother me. If I do however move the variable assignments of distance and distance2 to the top, my IDE warns me that

            if (o == null) 
              return 0;

Would then be 'dead' code. If that's the case, should null even be checked in this scenario?

What I mean is:

@Override
    public int compareTo(final Intersection o) {

        double distance = t;
        double distance2 = o.t;

        if (o == null) 
            return 0;

        if (distance == distance2) 
            return 0;


        return distance2 > distance ? -1 : 1;
    }

the return value 0 when one object is null seems wrong as the objects are not equal. I would prefer an exception in this case. — Henry, Dec 15 '14 at 16:58
It is dead code, because `o.t` would trigger a `NullPointerException` if `o` is null. Therefore the check for null is then not necessary anymore. If `o` is not null, then the check would also be false and "skipped". And yes, you should check for null. A `NullPointerException` is unnecessary here. — Tom, Dec 15 '14 at 16:58
I believe when eclipse comments that the section is dead code (this secion in particular) it means that based on the current source code and current execution paths, there's no way it can be null right now, as probably the only call to that method sends a non-null parameter. In the future however, you may call that method, or someone else may call that method, and pass in an argument that was passed down to it, that may be null. — Zymus, Dec 15 '14 at 16:58

score 15 · Accepted Answer · answered Dec 15 '14 at 16:57

15

Final may be null, it just means it cannot be assigned and is visible in anonymous inner classes etc.

In your case, the problem is different: (see comments)

public int compareTo(final Intersection o) {

    double distance = t;
    double distance2 = o.t; // here you use it, potentially causing NPE

    if (o == null) // if o was null, this is never reached
        return 0;

    if (distance == distance2) 
        return 0;


    return distance2 > distance ? -1 : 1;
}

answered Dec 15 '14 at 16:57

MightyPork

18,270
10
79
133

Yeah this does make much more sense. From a programmers point of view, should `null` be checked prior to invoking the method? I'm currently reviewing some old code and I'm somewhat boggled as to whether or not I should remove that condition completely. – Juxhin Dec 15 '14 at 17:01
In this case, this method shouldn't be invoked without being sure that the `Intersection` object isn't null – Juxhin Dec 15 '14 at 17:02
1

If you are sure enough that it won't be null, then there's no reason to keep the test. You might also want to have a look at this question: http://stackoverflow.com/questions/2858628 – MightyPork Dec 15 '14 at 17:04
@Juxhin "Shouldn't" doesn't mean "won't", though ;) – Dave Newton Dec 15 '14 at 19:54
@DaveNewton - Precisely, which is what triggered this whole dilemma. I know it shouldn't be invoked when `null` however how can I guarantee this? – Juxhin Dec 15 '14 at 19:55
2

@Juxhin You can use `@NotNull` if you're in an environment that supports it, @NonNull if you're on JDK 8, use an AOP-based contract system (like Contract4J), etc. I'd just check for null. – Dave Newton Dec 15 '14 at 20:00

score 2 · Answer 2 · answered Dec 15 '14 at 16:58

2

Your ide warns you because a final object can be null and you can get a null pointer exception on

double distance2 = o.t;

Hence the return statement or any statement inside the if o == null will never reach/execute

if (o == null) 
   return 0;

answered Dec 15 '14 at 16:58

sol4me

15,233
5
34
34

Makoto · Answer 3 · 2014-12-15T17:01:16.030

I'm not entirely sure why you don't think this is legal, or that this can't occur because your parameter is final:

yourClass.compareTo(null);

You should always check for null in scenarios that rely on the instance being present, preferably before you use the instance.

Marking the parameter final prevents you from actively changing the reference or mutating the value while in the method; it's a way to represent that this code is free of side-effects from the passed-in value.

Also, I do note an issue with your compareTo method; if the object you're comparing against is null, then the comparator denotes it as equivalent. You'll also run into edge cases with NaN and ==, as two double values might not exactly be equivalent.

You probably want something like this instead:

@Override
public int compareTo(final Intersection o) {

    double distance = t;
    double distance2 = o == null ? 0 : o.t;

    return Double.compare(distance, distance2);
}

score 1 · Answer 4 · answered Dec 15 '14 at 16:56

1

It's a reference to an object that is being passed and as such it can be null or not null regardless of weather it is final or not.

answered Dec 15 '14 at 16:56

David Soroko

8,521
2
39
51

If final object is being passed, should null still be checked?

4 Answers4