A very basic question about whether I should check for null and throw NPE? [duplicate]

Question

Consider the below method-

public void operationOnList(List<String> list) {
    list.add(1);
}

It is obvious that if list is null this method will throw a NullPointerException.

My question is should I instead check for null explicitly in this case?

public void operationOnList(List<String> list) {
    if (list == null) throw new NullPointerException("some message")
    list.add(1);
}

On one had I can add a message here, but what if I am not really concerned about the message? It helps me save an unnecessary branching in the code.

So which is preferable and which is more efficient?

EDIT - it is not a duplicate : In my case, the method will throw a NullPointerException if you pass a null to it in BOTH the above cases. I don't want or need to handle the exception and resolve it in the function.

What I am asking is should I check for null explicitly (adding unnecessary if condition or should I let the NPE occur when that variable is used?

Edit: -------------------------------------

A few thoughts over time:

1. If the method is exposed to client i.e. it is part of some api, it is a good idea to check as early as possible to check for the exception and throw an NPE with a good message. The reason being if the NPE gets thrown from further inside, an ugly stack trace gets printed, and user might not immediately know which method is the culprit.

Answer 1

I prefer NOT littering my code with null checks, because it's basically just clutter. Yes, sometimes it's necessary, like when reading user input or operating at application boundaries, getting stuff from third-party services etc.

But keep your business logic clean. If a parameter can't be null, trust that it isn't null. In fact, you should try to avoid or altogether ban null in your production code. If you never pass null, you will never get NPEs. This also lets you avoid initiating some dependencies in your tests that are not relevant for the current test.

Code that defensively checks for null everywhere is harder to read and harder to change, it makes the code more rigid and harder to test, and it's a really inefficient way of finding bugs.

Focus on making sure that values that go into your system get validated and "purified" and keep your core clean.

Answer 2

I normally perform these sorts of checks at the boundaries to sets of components e.g. for third party consumers. If you assert the incoming arguments for null-ness (and anything else) then your clients will get a clear message as to how they're misusing your API, and from that point on you can be reasonably safe in assuming that your input data is in a good and known state. Within the internals of my components I won't normally perform such checks.

In the above I might prefer an IllegalArgumentException, since that's what's being provided.

Nulls generally are a pain, since any reference can be a null, and you don't know which ones are 'valid' being null i.e. indicating that data is missing and that that needs to be catered for. If you genuinely are dealing with optional data, then your API for your clients and your underlying code will be clearer by using Optional. See here for more details.

Answer 3

IMHO, if it is for a library, I think it is OK to throw an exception so that the consumer of the library will know that something needs to be changed in the implementation of the method.

But if it is used for "final production" that will interact to the user, I think showing a message telling them that there is something wrong happening. After that the app should take whatever action is necessary based on the error.