CODEWITHSUNDEEP
javanullpointerexceptionnull
Martin Moore
Martin Moore

Reputation: 59

How to handle a null returned from a method?

I have a graph related method which returns the neighboring nodes of a certain node. If the one node has no neighbors it returns null, the method is the following

public Iterable<Node> getNeighbors(Node v) {
    if (!this.adjacencyList.get(v).isEmpty())
        return this.adjacencyList.get(v);
    return null;
}

I try to avoid the exception using the following :

if (graph.getNeighbors(nodeIterator.name) == null)
            nodeIterator = all_graph_nodes.iterator().next();
Iterable<Node> adjNodes = graph.getNeighbors(nodeIterator.name);

The NullPointerException is raised even using the previous code. How to solve this ?

Upvotes: 0

Views: 1652

Answers (3)

Drumstick
Drumstick

Reputation: 21

You should avoid returning null at all cost. This is of high danger as it may cause Null Pointer Exceptions to be thrown during runtime. Such exceptions are horrific to debug as they usually hide implementation errors due to the place where the exception was thrown is most likely far away from the original implementation error. Your case is actually a good example of such behavior as it is not directly understandable where the NPE is coming from.

In situations in which the appearance of a null value is inevitable (e.g. as @rzwitserloot pointed out, Java's Map get method) and there is the possibility of exposing it to client objects (e.g. your getNeighbors method may expose such null value) I like to use Java's Optional which (as stated in the docs) is:

A container object which may or may not contain a non-null value. If a value is present, isPresent() will return true and get() will return the value.

This object will act as wrapper to objects which may be assigned as null thus preventing it to be used directly and possibly preventing NPEs to be thrown.

In your case this would apply as follows (note that this is assuming that adjancencyList is a non-null object and that its get method is the one actually throwing the NPE):

public Optional<Iterable<Node>> getNeighbors(Node v) {
    return Optional.ofNullable(this.adjacencyList.get(v));
}

if (!graph.getNeighbors(nodeIterator.name).isPresent()) {
    nodeIterator = all_graph_nodes.iterator().next();
}

Iterable<Node> adjNodes = graph.getNeighbors(nodeIterator.name).get();

Note that by wrapping the original get method in an Optional object there is no longer the propagation of a raw null value hence preventing it to be used by the client. You are moving the responsibility of dealing with null to your side only and protecting clients to handle them instead.

Another great advantage of using Optional as a method's return type is that it implicitly declares that the return object of the method may or may not be present. This forces clients to understand that its return value may be empty (null) and thus force it to act accordingly.

Upvotes: 0

rzwitserloot
rzwitserloot

Reputation: 102872

If you're still getting an NPE, then the problem is in getNeighbours and not the second snippet.

  1. this.adjacencyList is null, -OR-
  2. this.adjacencyList.get(v) returns null.

Given that you're passing a name to a method that will then do a lookup by node, and that you can't call .get(someNodeRef) on a list, adjacencyList is probably some sort of hashmap, so your names are off and you should rename some things. Map's .get(x) method returns null if an entry is not found, so most likely the culprit is that v isn't in the map at all, and thus .get(v).isEmpty() throws NPE.

The fixes are as follows:

  1. You should NEVER return null when a valid sentinel value that carries the intended semantic meaning is available. A mouthful, but it means here: Why are you returning null when you intend to treat that the exact same way as 'zero nodes'? There is an instance of Iterable<Node> that properly represents the concept of zero nodes, and it isn't null. It's List.of() or equivalent: An empty list has no nodes. Great. That's what you intended. So return that.

  2. .get(v).isEmpty() is bad code here, as it would mean an NPE occurs if you ask for a non-existent node. Unless, of course, you want it to work that way. An easy way out is the defaulting mechanism: Call .getOrDefault instead:

if (!this.adjacencyList.getOrDefault(v, List.of()).isEmpty()) ....

except, of course, you should never be returning null when you can return an empty list instead, so your getNeighbours method becomes simply:

return adjacencyMap.getOrDefault(v, List.of());

that one-liner will fix all things.

In general, if you are writing code where null is dealt with in some way, and some sentinel value (such as a blank string or an empty list) is dealt with in the same way, your code is badly styled; however you got that null should have gotten you that empty value instead. e.g. if you ever write this:

if (x == null || x.isEmpty()) ...

you messed up. Figure out where you got x from. Update it there, make x the blank sentinel ("" for strings, List.of for lists, etcetera).

That, and use .getOrDefault and other such methods more: Methods that let you provide what should happen when e.g. a key is not found.

Upvotes: 1

JP Moresmau
JP Moresmau

Reputation: 7403

You should probably avoid returning null from your getNeighbors method. It's not good practice to return null for Iterables, Iterators and Collections, since an empty iterable would represent the same concept (there is nothing in that adjacency list) without all the dangers of null. And your code would be simpler. You can check if the iterable contains anything and if not then default to the full iterator.

Upvotes: 1

Related Questions