Why does this code sometimes throw a NullPointerException?

Question

Consider the following Java source:

if( agents != null ) {
  for( Iterator iter = agents.keySet().iterator(); iter.hasNext(); ) {
    // Code that uses iter.next() ...
    //
  }
}

The agents is a HashMap.

Why does the for statement sometimes throw a NullPointerException?

Thank you.

Answer 1

it's unlikey. post your full stack trace.

Answer 2

Shouldn't that be a while loop?

if (agents != null) {
    Iterator iter = agents.keyset().iterator();
    while (iter.hasNext()) {
        //some stuffs here
    }
}

or a for each?

if (agents != null) {
    //Assuming the key is a String
    for (String key : agents.keyset()) {
        //some stuffs here
    }
}

Answer 3

Thread Safety

If your code is multi-threaded, then it is possible. For example:

public class C {
  private Hashtable agents = new Hashtable();

  public iterate() {
    if( agents != null ) {
      for (Iterator iter = agents.keySet().iterator(); iter.hasNext();) {
        // Code goes here
      }
    }
}

If another thread sets agents to null immediately after the if statement executes (but before the for loop), then you will get a NullPointerException. Avoid this by using accessors (combined with lazy initialization).

Also, as others have mentioned, avoid such looping constructs in favour of generics, if possible. See other answers for details.

Accessors offer Protection

If you always use the following pattern you will never have NullPointerExceptions in your source code (third-party code, on the other hand, might have issues that cause your code to fail, indirectly, which cannot be easily avoided).

public class C {
  private Hashtable agents;

  private synchronized Hashtable getAgents() {
    if( this.agents == null ) {
      this.agents = new Hashtable();
    }

    return this.agents;
  }

  public iterate() {
    Hashtable agents = getAgents();

    for (Iterator iter = agents.keySet().iterator(); iter.hasNext();) {
      // Code goes here
    }
  }
}

The code that iterates over the agents no longer needs to check for null. This code is much more robost for many reasons. You can substitute Hashmap (or any other abstract data type, such as ConcurrentHashMap<K,V>) for Hashtable.

Open-Closed Principle

If you were feeling especially generous with your time you could go as far as:

public class C {
  private Hashtable agents;

  private synchronized Hashtable getAgents() {
    if( this.agents == null ) {
      this.agents = createAgents();
    }

    return this.agents;
  }

  public iterate() {
    Iterator i = getAgentKeyIterator();

    while( i.hasNext() ) {
      // Code that uses i.next() ...
    }
  }

  protected Hashtable createAgents() {
    return new Hashtable();
  }

  private Iterator getAgentKeyIterator() {
    return getAgentKeys().iterator();
  }

  private KeySet getAgentKeys() {
    return getAgents().keySet();
  }
}

This would allow subclasses (written by other developers) to substitute their own subclass of the abstract data type being used (allowing the system greater flexibility in keeping with the Open-Closed Principle), without having to modify (or copy/waste) your original work.

Answer 4

Make sure you don't set the iter to null inside the loop.