Checking for null parameter

[Daniel Pitts]

If an exception is thrown, runtime or not, it's a pretty darn good idea
that the calling code have a catch block for it, if one doesn't take the
care to pass only valid values in the first place.

I disagree. try/catch should be used only if you can handle the
exception, or wish to do some partial handling/translation. try finally
should be used to ensure invariants and cleanup when exceptions may
occur. Exceptions that are caused by incorrect usage should /_*NEVER*_/
be caught by the caller (perhaps by the uncaught exception handler, and
reported to the user in a friendly manor).

Incorrect API usage is a *bug*, and you should fix that bug, not fix the
symptom.

Given a method 'foo()' that can throw an IllegalArgumentException, the
best practice is to pass only valid values to it in the first place:

if ( arg == null )
{
handleNullSituation();
}
else
{
foo( arg );
}

NOW you don't need a try-catch, but if you don't do the pre-check, then
you absolutely would need to catch the exception.

I would say its a *bug* to not do the pre-check, or at least do an
assert. You still don't need to catch the exception.

To reiterate. The exception is reporting a bug. Don't shoot the
messenger, fix the bug instead.

 

[Daniel Pitts]

In the first place, *every* non-private constructor that
takes a reference parameter must work if the argument is null. Failure
to deal with a possible input is a flaw in the code - programmer error.

Define work? I think it is perfectly acceptable (and preferable) to have
a non-private constructor fail via throwing an exception if it cannot
initialize itself to its invariant.

In particular, dependencies that can't change over the lifetime of the
object *should* be checked at construction time to ensure correctness.
If you wait until they are used, then it becomes less clear who's bug
caused the problem. Remember, the sooner a bug is reported, the easier
it is to track down and fix.

 

[Daniel Pitts]

Best practice is to prevent errors. Failing that, catch them early.

Again, I disagree. Catch only what you can validly handle. If validly
handle means generating a bug report for a user to submit, then so be
it, but don't just catch exceptions willy-nilly.

"Just clutters the code" is not a valid criticism. If it is necessary
to check for an exception, clutter or not, it must be done.

Agreed, but there are somethings you can do, like combine try blocks.

The reason to check early is to facilitate correction of problems. The
later a bug is caught and logged, the harder it is to discern the true
cause.

Agreed, which is why you shouldn't catch and ignore, you should allow it
to propagate until it can be handled in a meaningful way (such as user
notification or program termination)

There are ways to make code easier to read and maintain, but the
sacrifice of good engineering principles is not among them.
Agreed.
To get "uncluttered" code, structure your algorithms so that non-nullity
of a variable is an 'assert'able invariant. NPEs will become
impossible, obviating the need for corresponding catch blocks. Then you
get your lack of "clutter" as a concomitant to good engineering, not a
violation thereof.

Agreed.

 

[Ulrich Eckhardt]

So, since 90% of the times I need a non-null object in my code, I
should throw an exception. So that means that 80% of any code that
calls a method or constructs an object that needs another object
should be inside a try catch.

No, that doesn't necessarily mean it. If it makes sense, you should verify
parameters given to your code, in particular when they come from other
code, e.g. when implementing a library. Otherwise, for debugging, I could
well live with the NPE that Java itself throws.

The problem is the complexity of the error handling. Consider the
following code:

public doSomething() throws Exception {
try {
Test t = new Test(this.aString);
...
} catch (Exception e) { }
// I don't care if Test threw an exception
try {
TestAgain a = new TestAgain();
...
// I do care if TestAgain threw an exception, so continue in the
try
// But I want to handle Tester exception differently
Tester ts = null;
try {
ts = new Tester(a);
} catch (Exception e) { throw e }
...
ts.doSomething();
}
}

You write that you don't care if 'Test' threw an exception, so I wonder why
you care at all if it was called? Typically, I have a series of calls
(constructor invocations or normal method calls) and one call provides data
for the next, so they are dependent. If the first one already fails, there
is no use continuing. Similarly, not every call has its own try-catch
clause but only all of them together, often not even there but rather
somewhere further up the call tree.

Honestly, I can't imagine a case where you have so much code where you don't
really care if it fails or not. What I do have sometimes is code that
silently swallows errors, e.g. when logging to a file but you don't have a
file attached I would simply ignore any logging request. However, this code
is then completely optional, i.e. the main code still works even without
it.

Oh, have in mind that this is more than I theoritical question than an
opinion. I'm not saying that null checking should be avoided.

This is a discussion about technical merits but also personal taste. I'm not
offended if someone disagrees with me.

Uli

 

[Daniel Pitts]

The nice thing about Java is it will check for null and throw an
exception without writing any code.. If all that is you are going to
do, there is not much point in manual checking. If however you can
dodge the null and carry on --where null means there is no such thing,
rather than a coding error, then of course you have to check for it.

You may want to check for null earlier and throw an NPE before that
point, if it will make the true source of the clearer. For example,
dependencies passed into a constructor should be checked for null, even
if they aren't used until later.

 

[Stefan Ram]

This is a discussion about technical merits but also personal taste. I'm not

The interface of a method »m( r )« might require »r« to refer
to an object of some kind. Then, in production code, I prefer
/not/ to check this within »m«. It is the client's duty.

For example, the client might call »m( x, r )«, where »r« must
not be »null«. I also assume that in the case that »r« is
null, the intended behavior is to do nothing. So, the client
might look like:

for( X x : c )if( r != null )m( x, r );

In such cases, the client can move the test out of the loop:

if( r != null )for( X x : c )m( x, r );

If the test would be part of »m( x, r )«, it will still be
needlessly performed now within »m«. So it will not be
possible to »move it out of the loop«.

Of course, during debugging, it will still help to enable
such redundant tests.

 

[Stefan Ram]

if( r != null )for( X x : c )m( x, r );
If the test would be part of »m( x, r )«, it will still be
needlessly performed now within »m«. So it will not be
possible to »move it out of the loop«.

PS: Possibly, some optimizers might be able to inline »m«,
and then move such a test out of the loop.

 

[Jack]

Lew a écrit :

Absolutely correct. Prevention is less expensive than cure.

Not so true. A bit OT, but Iraq was "preventive action" for Bush
government...

When you do catch exceptions, *log them*. Review your log file output;
your unit tests should force exceptions so that you can be certain that
your log messages actually are helpful. Generate lots of exceptions in
your tests, so that you can see what lots of log messages look like.

Seems quite a good advice.

For God's sake use a logging package, not System.err.println(), or the
really ridiculous System.out.println() for logging.

I thought you said that word like "ridiculous" was aiming the flame
rather than construct ?

Use 'assert', correctly, and you will prevent more bugs than exception
handling can ever fix.

It doens't fit my experience.

 

[Ulrich Eckhardt]

What constitutes and "exception that you can not handle"?

Simple example. You are parsing XML and putting it into a database. The
parser (SAX) calls a function to put a node into the database, but the
database signals an error. This error is propagated through the XML parsing
code but not handled there.

Lew:

Indeed. That is exactly the scenario to which I was providing an
alternative.

Your alternative doesn't make the code better, that's the problem. Also, you
can't proactively check the outcome of an operation before actually doing
it, so it can't even catch all possible errors. Consider the example I gave
with SAX. There you simply ignore exceptions that are not meant for you,
you just let them pass through your code, simple as that.

Uli

 

[John B. Matthews]

Lew a écrit :

Not so true. A bit OT, but Iraq was "preventive action" for Bush
government...

Just the opposite. They should have checked for null from getWMD(). ;-)

John

 

[pek]

Given a method 'foo()' that can throw an IllegalArgumentException, the best

practice is to pass only valid values to it in the first place:

  if ( arg == null )
  {
    handleNullSituation();
  }
  else
  {
    foo( arg );
  }

Firstly, I wonder what will handleNullSistuation() do other than
logging it and shouting "HEY! You have a bug here! Fix it!".

Secondly, let's name this method bar()... So
public void bar() {
if ( arg == null )
{
handleNullSituation();
}
else
{
foo( arg );
}
}

How is this any different than this:
public void bar() {
foo(arg);
}

In any of the above cases, a null arg IS WRONG! I mean, there is an
error in your code. What do you mean by preventing with this code?
What will handleNullSituation() do? Log it? You might as well read the
stackTrace.. You will find that arg is null, it's in the top of the
tree (assuming you are using some kind of IDE that, by clicking it,
you will be pointed right at it)!

 

[Tom Anderson]

Indeed it is. The cited link has no mention whatsoever of Java.

....

I know you're nuts, Lew, but are you serious? Do you understand what a
design pattern is? This is not something that's specific to java.

But, if you prefer examples in java, how about these:

http://www.cs.oberlin.edu/~jwalker/nullObjPattern/
http://www.owlnet.rice.edu/~comp212/00-spring/handouts/week06/null_object_revisited.htm

Just to be clear, i should point out that this is a digression - a Null
Object is not a 'physical' null, and doesn't involve the special
precautions we're discussing in the rest of the thread.

tom

 

[Tom Anderson]

Calling it "silly" is criminally irresponsible; someone might believe you.

A risk which, happily, does not exist concerning your advice that every
method null-check its parameters.

You must put try-catch blocks somewhere where exceptions can occur, or your
program will crash.


Wrong. All methods must be able to handle any input that they can receive,
otherwise your program will crash.

This all hinges on what you mean by 'can'. Consider this program:

public class LewIsWrong {
public static void printLength(String str) {
System.out.println(str.length()) ;
}
public static void main(String[] args) {
printLength("Hello, world!") ;
}
}

Would you say that an exception can occur in the body of printLine? Would
you say that printLine can receive a null as input?

If so, then this program does not meet your requirements, since it does
nothing about handling a null in printLength. And yet, it does not crash.

If not, then actually, we're in agreement.

This is a trivial example, and a matter of semantics, but it is not a
frivolous point. There can be as many places as you like in your program
where an exception, crash, etc could occur in certain circumstances, but
as long that those circumstances never arise, the program will not crash.
One way of preventing crashes is to remove the places where crashes can
occur; another is to stop them being triggered. The former sounds better
in theory, because it should be more 'robust', but it doesn't work in
practice - every method, no matter how trivial, would need to check its
arguments, and there would be as much argument-checking code as functional
code. Certainly, there are times when armouring a method is a good idea -
when it's an entry point to a package, or when it does something
potentially dangerous. It's simply not good engineering to do it in every
single public method, though.

I'm also curious about what you would suggest doing in printLength that
would prevent a crash when a null is passed in. Presumably, i test for
null, or catch the NullPointerException - but what do i do then? Skip the
printing? Print 0? Print an error message? I'd say that the program is
then behaving incorrectly, which is worse than crashing. Should it throw
an unchecked exception? Then it merely crashes in a different way. Should
it throw a checked exception? What could the calling code do with that
that isn't ultimately also crashing?

I don't know about you, but all the customers I've had consider letting
a program crash to be bad.

I'm with you here. I do not endorse programs crashing. I wish to make this
clear.

You say all public methods, but i [sic] think that's overkill. Firstly,
it only needs to be on exported methods - methods that you expect
outside code to call; the

Right, I did say this applied to public methods.

main entry points to the component or package or whatever. If outside
code wants to call your internal methods with duff arguments, fine, let
it handle the consequences. Secondly, i'm [sic] generally quite happy
to have

The consequences which are coded into the API and documented in its Javadocs,
yes.

code screws up when given a null, as long as it's clear that i [sic]
shouldn't pass a null. That again hands the responsibility for not
passing in a null to the caller.

It's only clear that you shouldn't pass 'null' to an API method if the
method planned for and handles the 'null', say by throwing an NPE,
No.

and tells you so in its Javadocs. Otherwise how would it be clear?

By telling you so in the javadocs.

It would be nice if java [sic] could express a requirement for non-nullity
in the type system. Some type systems forbid nulls from normal variables,

It can - by including a checked exception in the method signature.

No.

tom

 

[Lew]

A risk which, happily, does not exist concerning your advice that every
method null-check its parameters.

Correct, because I am advising good engineering practices.

And I didn't say that "every method [should] null-check its
parameters". I said that every method should (responsibly) handle
every possible input to it. Please do not use straw-man tactics.

This all hinges on what you mean by 'can'. Consider this program:

public class LewIsWrong {

Cute.

        public static void printLength(String str) {
                System.out.println(str.length()) ;
        }
                public static void main(String[] args) {
                printLength("Hello, world!") ;
        }

}

Would you say that an exception can occur in the body of printLine [sic]? Would
you say that printLine can receive a null as input?

That depends. If the class is exposed to other uses, then some future
client code very well could pass a null to printLength(). Just
because your main() didn't doesn't mean some other caller couldn't,
true?

To prevent that, you could have made printLength() private. Otherwise
you have *no* idea whether printLength() will ever receive a null
input. Therefore you must plan for the worst case - that it will.

If so, then this program does not meet your requirements, since it does
nothing about handling a null in printLength. And yet, it does not crash.

It does not crash if *and only if* called from the main() you show.
You do not show how someone else might call that method - their
program might crash.

In this scenario, printLength() must guard against null inputs,
perhaps by throwing an exception. To help future users of the class,
you should consider testing for and logging the error, or at least
creating a meaningful error message. At the very least, you should
document in the Javadocs that that method can throw an NPE. That
would be minimal handling of the null case.

If not, then actually, we're in agreement.

I do not know if we are in agreement until I see the Javadocs for the
printLength() method. If you document that it might throw an NPE,
then I agree that it's handled it.

This is a trivial example, and a matter of semantics, but it is not a
frivolous point. There can be as many places as you like in your program
where an exception, crash, etc could occur in certain circumstances, but
as long that those circumstances never arise, the program will not crash.

But you have not guaranteed that the circumstances cannot occur;
you've done nothing to prevent an NPE in the printLength() method.

In this case, assuming you document that the method can throw NPE, the
responsibility then falls on the client code to catch the exception
and log it.

One way of preventing crashes is to remove the places where crashes can
occur; another is to stop them being triggered. The former sounds better
in theory, because it should be more 'robust', but it doesn't work in
practice - every method, no matter how trivial, would need to check its
arguments, and there would be as much argument-checking code as functional
code. Certainly, there are times when armouring a method is a good idea -

This is correct, but you say it like it's a Bad Thing. Guarding
against incorrect input is one of the most powerful and fundamental
bug-stopping techniques there is. Quite frankly, I'm astounded that
you or anyone else would argue against it. No wonder so much
production code is so buggy.

when it's an entry point to a package, or when it does something
potentially dangerous. It's simply not good engineering to do it in every
single public method, though.

It's simply not good engineering to ignore bad inputs in any public
method.

I'm also curious about what you would suggest doing in printLength that
would prevent a crash when a null is passed in. Presumably, i [sic] test for

I don't suggest that. Why do you think I do?

What I do suggest is that the caller of the method make sure that it
not pass in a null.

null, or catch the NullPointerException - but what do i do then? Skip the
printing? Print 0? Print an error message? I'd say that the program is
then behaving incorrectly, which is worse than crashing. Should it throw
an unchecked exception? Then it merely crashes in a different way. Should
it throw a checked exception? What could the calling code do with that
that isn't ultimately also crashing?

You are arguing against points that I have not made and with which I
do not agree. Why would I defend points with which I do not agree?

I'm with you here. I do not endorse programs crashing. I wish to make this
clear.

If you don't want the program to crash, then you must intercept and
deal with any event that would otherwise cause a crash. Apparently we
are in agreement in the end.

No.

"No" what? Is that supposed to be a point?

and tells you so in its Javadocs.  Otherwise how would it be clear?

By telling you so in the javadocs.

It would be nice if java [sic] could express a requirement for non-nullity
in the type system. Some type systems forbid nulls from normal variables,

It can - by including a checked exception in the method signature.

No.

Again, what is that supposed to argue? That declaring a checked
exception does not provide a way to express such a requirement? But
it most surely does. If you have some other point, then you should
make it clear. Maybe add a subject, verb and object to your sentence?

 

[Lew]

...

I know you're nuts, Lew, but are you serious? Do you understand what a

How about a little break from the pointless /ad hominem/ arguments?

design pattern is? This is not something that's specific to java.

No, but the discussion here is, and the cited link was not relevant to
the discussion here.

As you yourself pointed out, it's a different subject altogether. The
"Null Object" in the wikipedia link has nothing to do with the fact
that in Java, no object can be null.

Whether I am nuts or not.

Just to be clear, i should point out that this is a digression - a Null
Object is not a 'physical' null, and doesn't involve the special
precautions we're discussing in the rest of the thread.

Again, confirming my point that the referenced article is not about
the Java 'null' that was the topic of this conversation.

Are you just picking fights to be ornery? You pretend to disagree,
then confirm the point I made.

 

[pek]

I'm also curious about what you would suggest doing in printLength that

would prevent a crash when a null is passed in. Presumably, i [sic] test for
null, or catch the NullPointerException - but what do i do then? Skip the
printing? Print 0? Print an error message? I'd say that the program is
then behaving incorrectly, which is worse than crashing. Should it throw
an unchecked exception? Then it merely crashes in a different way. Should
it throw a checked exception? What could the calling code do with that
that isn't ultimately also crashing?

You are arguing against points that I have not made and with which I
do not agree. Why would I defend points with which I do not agree?

That is not an answer to his question. What would you do in this
situation? I am also curious. His point, I believe and agree, is
either way you choose to go, you still have to fix the point at which
a null reference is passed, thus, checking for it isn't going to
change anything.

 

[pek]

I'm also curious about what you would suggest doing in printLength that
would prevent a crash when a null is passed in. Presumably, i [sic] test for
null, or catch the NullPointerException - but what do i do then? Skip the
printing? Print 0? Print an error message? I'd say that the program is
then behaving incorrectly, which is worse than crashing. Should it throw
an unchecked exception? Then it merely crashes in a different way. Should
it throw a checked exception? What could the calling code do with that
that isn't ultimately also crashing?
You are arguing against points that I have not made and with which I
do not agree. Why would I defend points with which I do not agree?

That is not an answer to his question. What would you do in this
situation? I am also curious. His point, I believe and agree, is
either way you choose to go, you still have to fix the point at which
a null reference is passed, thus, checking for it isn't going to
change anything.

Your statement makes no sense. How can you fix what you can't detect? In
order to fix the null reference, you have to find it, which means you have to
check for it. How else do you find it?

What is the difference of reading what you logged and reading the
stack trace of a NPE? In both cases, you can find it.

I suggest that one catch and log the exception. If you prevent the exception,
which is usually better, then catch and log the illegal situation such as null
input. Then restore the program to valid state, commonly a retry of the
input. Is that not the obvious approach?

How do you prevent an exception? By checking if the object is null
before calling a method? How is that preventing? What will it do after
not calling the method? Inform the user for an abnormal situtation and
then close? Isn't this the same thing as if a NPE was thrown? Why even
bother? Because in any case, your code has a bug. Either way, the
program must end because the method wasn't executaded as it had to.

 

[pek]

Tom Anderson:


Printing an error message when there's an error is incorrect behavior? In
what universe?

That is not the point. Printing or not printing the error doesn't
change a thing. The program will exit either way.
If you want the program to exit with a pretty message you can catch
any uncatched exceptions from the programs main method and log the
stack trace. The program will still HAVE TO EXIT.

 

[pek]

Sample recovery dialog:

"We're sorry, but remote document storage seems to be unavailable. Would you
like to store your document locally until the remote document storage is back?
(Y/n) _"

What's that? Where is the NPE in that code? That looks like a generic
error that would be naturally thrown if a connection couldn't be
established. You know that this will happen.

public void foo() {
try {
bar.connect();
} catch (ConnectionException e) {
...
}
}

I'm not against cathcing any exceptions. A NPE exception would be if
bar was null. That would mean that somewhere in your code you forgot
to instantiate bar, so now the connect() method will NEVER work,
because you have a bug in your program. So.. How about:

public static void main(String[] args) {
try {
startProgram();
} catch (NullPointerException e) {
logger.log(e);
//Show message "We're sorry, but there was an error in the
program" (or a better one, I'm not good at words)
}
}

public void startProgram() {
...
// Do anything
// At some point you have to connect
try {
bar.connect();
} catch (ConnectionException e) {
// Show message "We're sorry, but remote document storage seems to
be unavailable. Would you like to store your document locally until
the remote document storage is back? (Y/n) _"
}
}

Now the program will exit and log it.

 

[Jack]

Lew a écrit :

Lew a écrit :


The practice is ridiculous - that's an engineering assessment. Note
that the adjective is not applied to any individual, hence not flame.


How have you used 'assert', and how has it failed you?

This is an important inquiry. 'assert' is a very powerful tool to
enforce program correctness. Cases where it doesn't help *could be* due
to ineffective use of it.

Well basically, i used assert to check various invariants for bad reasons:
1/ In "not yet finalized" mode, to check that the invariants i thought
was there, was really there. I would say it is ok, but not more than:
if( !invariant()) throw ...

2/ After putting my GUI component in a new place of the GUI, i got
exceptions because of default initiation wich was not planned. So i had
to remove my invariants, and all the assert (i wasn't sure of the bad
invariants anymore).


So now i check for local condition in every method, and i don't assume
any invariant wich can't be enforced by java code (a final field wich is
initialized CANNOT be null).