2007-03-05

Beware the hashcode!

(With thanks to my colleague Armand "jojo" Dijamco)

What does this code print?

Map map = new HashMap();
    
Collection things = new HashSet();
things.addmap );
System.out.printlnthings.containsmap ) );
    
map.put"foo""bar" );
System.out.printlnthings.containsmap ) );

Be wary of storing mutable objects in a HashSet, or using them as keys in a HashMap. An implementation side effect of using a collection that relies on hash codes is that making changes to the object in a way that alters its hash code will cause it to no longer be considered the same object. There's no way (short of removing and re-adding the item from the collection) to inform a collection that a mutable object needs to be re-hashed.

Item 13 in Josh Bloch's Effective Java tells us to "Favor Immutability". He also tells us in item 8 to "Always override hashCode when you override equals". A corollary is that you should not use mutable objects as keys in HashMaps or values in HashSets. If such objects correctly implement hashCode, they're not reliable in hashcode-based collections.

AddThis Social Bookmark Button

2007-03-03

Unit testing Swing components - impossible?

Testing Swing components seems to fill many developers with fear. "How can I unit test this thing?" they often ask, "it's full of icky event handling code and troublesome painting...".

Frameworks such as Abbot are often the solution developers find for this problem. Essentially, many developers abandon the idea of writing regular unit tests for Swing components, and resort to "click simulators" which are frequently functional tests rather than unit tests.

There is a lot you can do just to test regular Swing components using bog-standard JUnit (or Test-NG) tests. Consider the following fairly basic Swing component.

package org.dubh.blog;

import java.awt.Graphics;

import java.awt.event.MouseAdapter;

import java.awt.event.MouseEvent;

import java.util.List;

import java.util.concurrent.CopyOnWriteArrayList;

import javax.swing.JComponent;

/**
 * A user interface widget.
 */
public final class Widget extends JComponent {
  
  private List<WidgetListener> _listeners = 
    new CopyOnWriteArrayList<WidgetListener>();
  private String _label = "";
  
  public Widget() {
    // Just for demonstration purposes.
    addMouseListenernew MouseAdapter() {
      public void mouseClickedMouseEvent e ) {
        if e.getButton() != MouseEvent.BUTTON1 return;
        fireWidgetClicked();
      }
    });
  }
  
  /**
   * Adds a listener. 
   
   @param wl listener to add. Must not be null.
   * @category Event handling
   */
  public void addWidgetListenerWidgetListener wl ) {
    if wl == null throw new NullPointerException"wl is null" );
    
    _listeners.addwl );
  }
  
  /**
   * Removes a listener.
   
   @param wl listener to remove.
   * @category Event handling
   */
  public void removeWidgetListenerWidgetListener wl ) {
    _listeners.removewl );
  }
  
  /**
   * Notifies listeners that the widget was clicked.
   
   * @category Event handling
   */
  protected void fireWidgetClicked() {
    if _listeners.isEmpty() ) return;
    
    WidgetEvent event = new WidgetEventthis );
    for WidgetListener l : _listeners ) {
      l.widgetClickedevent );
    }
  }
  
  /**
   * Sets the label of the widget.
   
   @param label a widget label. Null is allowed, will be 
   *    converted to the empty string.
   */
  public void setLabelString label ) {
    if label == null label = "";
    if label.equals_label ) ) return;
    
    String oldLabel = _label;
    _label = label;
    repaint();
    firePropertyChange"label", oldLabel, label );
  }
  
  /**
   * Returns the label of the widget. 
   
   @return the label of the widget. Never returns null.
   */
  public String getLabel() {
    return _label;
  }
  
  @Override
  public void paintComponent(Graphics g) {
    g.drawString_label, 0, getHeight() );
  }
}

We can start off with the really simple stuff. Let's at least test that we can make new instances of Widget.

  public void testConstructor() {
    new Widget();
  }

Seems pretty dumb? Well... that constructor contains code, so maybe it could throw an exception. It'd be good to catch this when running our tests rather than when the code is out running in production. If you can do nothing else, at least constructing a class in a test is better than no test at all. But wait! There's a lot more we can do here. What about testing the pretty boring setLabel() / getLabel() set?

  public void testGetAndSetLabel() {
    Widget widget = new Widget();
    // Test the initial state.
    assertEquals"", widget.getLabel() );
    // Test setting and getting.
    widget.setLabel"Hello!" );
    assertEquals"Hello!", widget.getLabel() );
    // Test edge case ( null label )
    widget.setLabelnull );
    assertEquals"", widget.getLabel() );
  }

OK.. this is a bit more meaty. We're testing some real assertions here. Now for the "hard" stuff. How can we test events? Well... Ignore the fact that these are events for now, and just look at the code for what it is. The management of listeners and the sending of events doesn't actually require AWT in most Swing components. Even if it did, we're not testing AWT here, we're testing Widget.

Let's use a common technique for testing listeners - introduce an inner class that provides a stub listener which simply tracks the events it receives:

  private class WidgetListenerStub implements WidgetListener {
    List<WidgetEvent> receivedEvents = new ArrayList<WidgetEvent>();

    public void widgetClicked(WidgetEvent e) {
      receivedEvents.add);
    }
  }

Now testing that we can add and remove widget listeners and receive events is easy:

  public void testWidgetListeners() {
    Widget widget = new Widget();
    
    // Test firing when there are no listeners.
    widget.fireWidgetClicked();
    
    WidgetListenerStub l = new WidgetListenerStub();
    widget.addWidgetListener);
    
    widget.fireWidgetClicked();
    assertEquals1, l.receivedEvents.size() );
    assertSamewidget, l.receivedEvents.get).getSource() );
    
    widget.removeWidgetListener);
    widget.fireWidgetClicked();
    assertEquals1, l.receivedEvents.size() );
  }

OK... what hasn't been tested yet? Well, we probably want to test multiple listeners and property change event notifications. Those are a pretty easy extrapolation from the example above.

One thing we haven't tested yet is that actually clicking in the widget with the mouse causes WidgetEvents to be fired. To do this, we don't have to actually test that a low level operating system mouse click event will cause our event to be fired. In other words, again, we're not testing AWT. We trust that Sun did a good job testing AWT already. No Robots required. Let's focus on events just at the level of our component, using a bit of reflection magic:

  public void testMouseClickTriggersEvent() throws Exception {
    Widget widget = new Widget();
    WidgetListenerStub stub = new WidgetListenerStub();
    widget.addWidgetListenerstub );
    
    // Simulate a mouse click using reflection.
    MouseEvent event = new MouseEventwidget, MouseEvent.MOUSE_CLICKED, 0
      MouseEvent.BUTTON1_MASK, 10101false );
    Method process = Component.class.getDeclaredMethod
      "processEvent", AWTEvent.class );
    process.setAccessibletrue );
    process.invokewidget, event );
    
    assertEquals1, stub.receivedEvents.size() );
    
  }

We're sending a mouse event directly to our widget component, and verifying that it causes the Widget to fire a WidgetEvent as expected. The really cool thing about this is that this does not require the component to be visible on screen, and it doesn't require native AWT event handling. Indeed, this test works perfectly fine even in headless mode (when the system property java.awt.headless=true, or you're running on a system with no graphics display).

One last thing we haven't tested: paintComponent(). This is arguably the toughest thing of all to test. Like the constructor example, it's at least a good idea to call this method in a test and make sure no exceptions occur. We can go a little bit further and actually test that paintComponent really painted something. More on that in my older blog entry, Unit Tesing: Coverage in Those Hard to Reach Places.

While frameworks like Abbot have their place, it's a lot easier to test Swing components with regular JUnit or Test-NG tests than you might think.

AddThis Social Bookmark Button

2007-03-02

What's up with the 2.0?

Well... If I was being more accurate, this would really be 4.0.

In the beginning, there was Brian Duff's Weblog over on Radio Userland (good grief, that wasn't even Web 1.0. You had to install a thick client application just to post. Yowzers), then I set up Orablogs, and moved over to there. Until fairly recently, I blogged (or rather, mostly didn't out of laziness) at blogs.oracle.com.

So why move now?

Well. I got a wee bit frustrated with the blog management software I was using - it had a troublesome tendency to mangle Java code in particular, which is something of a pain when you're trying to write a Java coding related blog. So far, I'm pretty happy with blogger.com's interface.

Another reason for the move is that I want to blog about more random stuff outside the world of Oracle. For instance, if I decide to play with Eclipse or JBoss (perish the thought), blogs.oracle.com doesn't really feel like the appropriate place. Even though my blog there tries not to be, like most corporate blogging sites, blogs.oracle.com suffers from the perception that the bloggers thereon are corporate marketing drones. OK, it's not just a perception - it might just about be true in some cases... .

A third reason is that I've owned this domain name (dubh.org) since about the time I finished university, and never did anything useful with it. Now, thanks to Rob, I'm totally in love with Google Apps and am using it to host all manner of fun stuff at the dubh.org domain. By the way, feel free to drop me an email at brian@dubh.org to say hi.... good grief... it's liberating being able to post mailto: links again safe in the knowledge that gmail's spam filters will deal with most of the crud ;).

Having a blog on my own domain also means that should I (perish the thought) ever move on to new pastures, I don't have to say adios to all of my blog content...

AddThis Social Bookmark Button

2007-03-01

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 :)

AddThis Social Bookmark Button