avoiding "Potential null pointer access"

Question

which would you say is the best practice when implementing the following problem:

MyClass myVariable = null;
if ( ..condition 1.. ) {
  myVariable = new MyClass(1);
} else if ( ..condition 2.. ) {
  myVariable = new MyClass(2);
}

myVariable.execute();

Which would be a good solution to the warning?

A finishing else

final MyClass myVariable;
....
} else {
  // let's say this assert makes sense here
  Assert.fail("This should not happen");
}

throw RuntimeException

final MyClass myVariable;
....
} else {
  throw new RuntimeException("Some message, like <should not happen>");
}

Check for NPE

final MyClass myVariable;
....
if (myVariable != null) {
  myVariable.execute();
}

Other ideas?

Thanks in advance!

score 1 · Accepted Answer · answered Jan 29 '15 at 09:44

It depends on whether either condition 1 or condition 2 must always be true. If condition 2 is the exact opposite of condition 1, you can replace the else if ( ..condition 2.. ) with else and solve your problem.

If it's not, and the fact that both condition 1 and condition 2 are false indicates some invalid input, I'd throw an exception.

If the scenario in which both conditions are false is a valid scenario, I'd check that myVariable is not null before calling myVariable.execute().

score 0 · Answer 2 · answered Jan 29 '15 at 09:42

0

Better is the following.

final param;
if ( ..condition 1.. ) {
  param = 1;
} else if ( ..condition 2.. ) {
  param = 2;
} else {
   throw new IllegalArgumentException("no condition matches");
}

new MyClass(param).execute();

If your conditions are simple try to re-write if-else chain using switch-case. It is better.

answered Jan 29 '15 at 09:42

AlexR

114,158
16
130
208

You should only throw `IllegalArgumentException` if there an illegal *argument* to the method being called. There is no indication in the context that `condition 1` and `condition 2` are validating arguments. – Stephen C Jan 29 '15 at 09:48
@StephenC, I agree with your statement. I however absolutely sure that OP's "condition" depends on argument that can be mentioned in exception. If this is wrong `IllegalStateException` sounds better. The point in my solution is that (1) throw exception is something is going wrong and (2) minimize code duplication: there is only one constructor call. – AlexR Jan 29 '15 at 09:52
You are (in effect) recommending that the OP throws a specific exception, when he is already proposing to throw `RuntimeException` or an `AssertionError`. That is only a good recommendation if the specific recommended exception is *more* appropriate than one of the ones he / she is already considering. – Stephen C Jan 29 '15 at 09:55
Besides, I can see zero evidence in the question to support your assumption / "absolute sureness". – Stephen C Jan 29 '15 at 10:12

score 0 · Answer 3 · answered Jan 29 '15 at 09:55

I would do a null-check, like your 3rd example. If the feature you are implementing is a side-feature or something optional, you can probably ignore it when your variable is null. When you have a main feature 'though, and the user awaits response, you should then message the user and ask him to try again.

Giulio Biagini · Answer 4 · 2015-01-29T10:08:22.977

If you can always initialise your variable, use an if-else which ends with an else:

MyClass myClass;

if (...)
    myClass = new MyClass(1);
else if (...)
    myClass = new MyClass(2);
else if (...)
    ...
else
    myClass = new MyClass(n);

myClass.execute();

If you can't always initialise your variable and you want to use the variable only when initialised:

MyClass myClass;

if (...)
    myClass = new MyClass(1);
else if (...)
    myClass = new MyClass(2);
else if (...)
    ...

if (myClass != null)
    myClass.execute();

If you can't always initialise your variable but it is needed:

MyClass myClass;

if (...)
    myClass = new MyClass(1);
else if (...)
    myClass = new MyClass(2);
else if (...)
    ...
else
    throw new Exception(...);// or notify the user and exit

myClass.execute();

Another approach is to define an init() method:

MyClass myClass = init(...);

// check if myClass is != null if init can return a null
myClass.execute();

MyClass init(...) {
    if (...)
        return new MyClass(1);
    else if (...)
        return new MyClass(2);
    else if (...)
        ...
    else
        return new MyClass(n);// or return null

In short, it depends on the case you are.

Stephen C · Answer 5 · 2015-01-29T10:06:27.170

which would you say is the best practice when implementing the following problem

It depends on the context:

It depends on the (deep) purpose of checking condition 1 and condition 2.
It depends on what it means for them to both be false:
- Is it a "normal" condition?
- Is it a user input error?
- Is it a programming error in the code that called this method?
- Is it a programming error in this code; e.g. a violation of an invariant?
- Something else?
It depends on how you want that case to be treated.

Depending on that, any of your proposed alternatives could be appropriate.

... which would you say is the best practice ...

I would not use the phrase "best practice" for something like this. Not even if I knew the context. Whenever I hear the phrase "best practice" in a question, I get the impression that someone wants a "cookie cutter" solution that they can apply without thinking.

The only "best practice" I would recommend here is to understand the context, and pick the solution most appropriate to it.

And ... "any solution that makes the warning go away" is NOT the right approach.

avoiding "Potential null pointer access"

5 Answers5