Skip to main content

Check parameters for validity... good advice!

I recently ran into a weird NullPointerException in part of our code:

Exception in thread "main" java.lang.NullPointerException
 at org.dubh.blog.Graph$Vertex.access$000(Graph.java:17)
 at org.dubh.blog.Graph.getVerticesConnectedTo(Graph.java:14)
 at org.dubh.blog.Graph.main(Graph.java:26)

Hmm... access$000? That's odd. The code looked something like this:

01 package org.dubh.blog;
02 
03 import java.util.ArrayList;
04 import java.util.HashMap;
05 import java.util.List;
06 
07 final class Graph
08 {
09   private HashMap _vertices = new HashMap();
10 
11   public List getVerticesConnectedToObject vertexKey )
12   {
13     Vertex vertex = (Vertex_vertices.getvertexKey );
14     return vertex.fromEdges;
15   }
16 
17   private final class Vertex
18   {
19     private final List fromEdges = new ArrayList();
20   }
21   
22   public static void mainString[] args )
23   {
24     Graph graph = new Graph();
25     graph.getVerticesConnectedTogetSomeVertex() );
26   }
27 }

Can you tell quickly from looking at these two pieces of information what the problem is?

It turns out that getSomeVertex() was returning null, and this was being passed in as the value of vertexKey. Unlike its older cousin, Hashtable, which would throw a NullPointerException if you tried to get null, HashMap.get( null ) returns null, so the vertex variable in getVerticesConnectedTo() was also null.

OK... so why did the NullPointerException happen on line 17 (which is the inner class declaration), instead of line 14, which is where we're actually dereferencing the null variable? Well, it's because the Java compiler creates a super-secret static method for accessing the fields of inner classes. With the geeky power of javap, you can see it being called in the bytecode:

public java.util.List getVerticesConnectedTo(java.lang.Object);
  Code:
   0:   aload_0
   1:   getfield        #4; //Field _vertices:Ljava/util/HashMap;
   4:   aload_1
   5:   invokevirtual   #5; //Method java/util/HashMap.get:(Ljava/lang/Object;)L
java/lang/Object;
   8:   checkcast       #6; //class org/dubh/blog/Graph$Vertex
   11:  astore_2
   12:  aload_2
   13:  invokestatic    #7; //Method org/dubh/blog/Graph$Vertex.access$000:(Lorg
/dubh/blog/Graph$Vertex;)Ljava/util/List;
   16:  areturn

If the generated method were written in Java it would look something like this:

private final class Vertex
{
  private final List fromEdges = new ArrayList();

  private static List access$000Vertex instance )
  {
    return instance.fromEdges;
  }
}

Now the reason for the NPE happening before vertex is dereferenced on line 14 is pretty obvious.

Two lessons from this:

1) Follow Josh Bloch's Advice in Effective Java - "Item 23: Check parameters for validity"

A simple null check in the getVerticesConnectedTo method would have produced a much more readable exception:

public List getVerticesConnectedToObject vertex )
{
  if vertex == null throw new NullPointerException"vertex is null" );
  ...
}

Yields:

Exception in thread "main" java.lang.NullPointerException: vertex is null
 at org.dubh.blog.Graph.getVerticesConnectedTo(Graph.java:13)
 at org.dubh.blog.Graph.main(Graph.java:27)

2) It's almost always better to access fields using method calls. Even in private inner classes.

It would have been better if the Vertex inner class above had provided an accessor method for the fromEdges field. If it had, the exception would have been clearer. A few people worry about the performance of method calls vs. field access. Remember that performance solutions aren't solutions until you can prove as much using a profiler, and that the point is somewhat moot in the above case, since there's effectively a static method call rather than a getfield in the generated bytecode anyway, canceling out the programmer's misguided attempt to save cycles and / or typing. Cough... the programmer in this case, of course, being me :)

Comments

  1. For the first advice about adding null checks, I agree that it would help out in debugging and identifying the problems quickly but would it be a good practice to add code (in our case if checks) which would get executed only once or to say it in different words is it a good practice to add code because the caller is not obeying the api contract which is clearly mentioned in the java docs?

    ReplyDelete

Post a Comment

Popular posts from this blog

Java Blooper #2: Must be a Better Way...

The post you're reading is ancient, and yet slightly inexplicably popular :) I've recently started blogging again in 2020 with some fresh content. Check out some of the new topics about blogging again, dynamic method invocation, and aapt2.It's Monday, which means it's time for another blooper... What's wrong with this code? boolean hasThing( List things, Thing thing ) { for ( int i=0; i < things.size(); i++ ) { if ( thing.equals( things.get( i ) ) ) { return true; } } return false; } Update: Minor edit to add missing parenthesis from if statement that got "lost in translation" en-route to the blog :)

Configuring Mac OS X Terminal

The post you're reading is ancient, and yet slightly inexplicably popular :) I've recently started blogging again in 2020 with some fresh content. Check out some of the new topics about blogging again, dynamic method invocation, and aapt2.I recently installed Leopard (Mac OSX 10.5) on a new mac. There are a few factory settings I usually change on a new installation, although by far fewer than I do typically with Windows. One of them is the default keyboard configuration for Ctrl+Left Arrow, Ctrl+Right Arrow, Page Up, Page Down, Home, and End in Terminal. The default settings drive me a bit potty since I'm used to using Linux and emacs every day at work.Changing them is easy, fortunately. Just visit Keyboard under Settings in Terminal->Preferences, and modify the following keys, so that their action matches the value shown. You can edit the keystroke for an item by double clicking on it, selecting "send string to shell", and typing the indicated keys.KeyActio…

Java Blooper #1: Ternary Insanity

The post you're reading is ancient, and yet slightly inexplicably popular :) I've recently started blogging again in 2020 with some fresh content. Check out some of the new topics about blogging again, dynamic method invocation, and aapt2.From time to time, we all write code that could be clearer. Sometimes in the rush of solving a problem, we don't pay attention to the micro details of the code flowing from our fingertips. Other times, we refactor some existing code, and don't necessarily take the opportunity to clean up as much as we could.I find it useful sometimes when reading code to think about whether it could be rewritten in a more straightforward way, and if so whether any lessons can be learned about writing tight and expressive, and most importantly, readable code.Over the next few weeks, I'm going to blog weekly examples of some Java code bloopers that I've seen. All the examples are real and have been observed "in the wild". However some…