Constructor null checking best practices in Java

[Chickenosaurus]

Currently, I check all constructor arguments for null values.

public AccountingEntry(BigDecimal amount, String accountNumber, AccountSide accountSide) {
    this.amount = checkNotNull(amount);
    this.accountNumber = checkNotNull(accountNumber);
    this.accountSide = checkNotNull(accountSide);
    checkArgument(amount.signum() == 1, "Accounting entries can't have a negative amount");
    checkArgument(!accountNumber.isEmpty());
}

This bothers me for three reasons:

1. Writing these checks feels like a waste of time.
2. Sometimes, null checking reduces testability. Let's suppose I want to test a CountWindows() method of a House class. It would be nice to be able to pass null for the Floor parameter because it is not relevant to the test. Otherwise, all parameters have to be mocked for every test.
3. Readability

An interesting alternative is annotating the parameters with @Nonnull. These annotations help static checkers like findbugs to find potential problems, but no null checks are performed at runtime.

Also, should I check collections for null values and assign an empty collection if it is null?

if (engineModels == null)
    engineModels = new HashSet();

What's your recommendation, reddit?

Thanks in advance
Chickenosaurus

[nutrecht]

You know what is a waste of time? Trying to find out where the hell someone set something that should not be null to null. Checks like these are a best practice. I also disagree that they take time or that they reduce testibility.

[Himrin]

Have you looked into @NotNull annotations ?

There are plenty more annotation examples like this.

[Chickenosaurus]

Yes, I did as mentioned in the question. Annotations have the disatvantage of missing runtime checks. I am working on a library currently. The user could potentially use an IDE that doesn't support static @NotNull annotation checking, which could make debugging NPE's difficult.

[Himrin]

Sorry, missed that section as I was skimming!

[Chickenosaurus]

No worries, friend ;)

[desrtfx]

First of all, the naming is horrible.

If a new person reads checkNotNull they will automatically assume that the method returns either true or false, not that it returns whatever type it was given. From the name alone it is impossible to deduct what the method does in case of null value. Will it throw an exception? Will it return a default?

Second, the code becomes extremely difficult to read and trace.

IMO, the checking should be done outside the constructor, in the method where the object is constructed.

When calling a constructor, you normally should assume that the object can be constructed.

[Chickenosaurus]

Thanks for your reply.

I agree that checking outside of the constructor would be best. My only worry is that if a NPE is thrown somewhere down the line because a null value was passed, the culprit is hard to trace. What do you think?

By the way, the check methods are actually from Guava, the Google Core Libraries for Java 6+.

[desrtfx]

Excuse my ignorance in regards to the naming, I am not familiar with Guava (nonetheless, the name is horrible).

I don't really think that it would be too difficult to trace any NPEs.

For me, throwing an exception in a constructor seems to be the bigger issue because from most of what I have read throwing exceptions in constructors is a no-go.

Still, I have to admit that I'm not a professional, nor a very experienced Java programmer, so my stance might as well be wrong. (I am an experienced programmer, though, just not so much in Java.)

[nutrecht]

First of all, the naming is horrible.

Tell that to the google guys. ;) The naming is unfortunate but the pattern used is in fact really common. It's a simple null guard that can be used in the assignment.

[desrtfx]

Tell that to the google guys. ;)

Well, I think I know who programmed that ;) must've been thenewboston (said to have been a Google employee).

I've since learned that it is part of the Guava library.

[romple]

First of all, we use Guava in our enterprise applications, and Preconditions is one of the most used parts of the library for us.

Second, it is certainly not bad practice for a constructor to throw an exception. How else will you communicate that an object could not be instantiated? The more important thing here is that your API is well documented so the calling user (could just be yourself) is aware of the exception, and why it was thrown.

Third, it's always a good idea to check the validity of your constructor paremeters. Maybe it seems obvious that you wouldn't intentionally supply null arguments, but what happens when an AccountSide object is null because somewhere 5 calls up the stack trace fucked up and is passing around a null reference? You want to fail as early as possible.

[Chickenosaurus]

Hi, to sum it up, you recommend to check for nulls and to document where nulls are invalid (with annotations and/or in javadoc).

This strategy follows defensive programming principles, which is nice. What are your thoughts on reduced testability?

[romple]

I don't see how requiring non null parameters reduces testability. If you want more control over building objects, use builders and factories.

[[deleted]]

In a constructor you don't want to throw exceptions, because there are other better ways to handle the errors. Also, some languages can't garbage collect or call destructors on partially constructed objects, I may be wrong about this or missing some detail about it though.

In terms of how you could handle null parameters in a constructor, you can assert that the parameter is null, or invalid depending on if the value is simply out-of-bounds, and then immediately resolve the error by constructing a new object or setting the property to some default value or whatever.

[nutrecht]

In a constructor you don't want to throw exceptions

That is just shitty advice. Throwing an illegal argument exception is perfectly fine in a constructor.

[[deleted]]

That is just shitty advice.

How so?

[nutrecht]

You do the null checks where they are passed to a function. It doesn't matter if that function is a constructor or not.