Even in the jdk there is bad code.

Java 7, TreeSet and NullPointerException.

Recently I tried to compile with java 7 a project developed with java 6.

Lot of fun happened during tests execution, tests that in java 6 were  running smoothly, with java 7, they were strangely failing!
So, I had to understand why and this is what I discovered...

The context first:
In that project I have a simple Hibernate Entity more or less like the following. 

package com.marco.test;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.Table;
import javax.persistence.UniqueConstraint;
import org.hibernate.validator.NotNull;
@Entity
@Table(...)
public class ABean {
        
        ...
        
        private String name;

        @Column(name = "name", nullable = false)
        @NotNull
        public String getName() {
                return name;
        }

        public void setName(String name) {
                this.name = name;
        }
}

note that the field "name" is nullable=false and marked with @NotNull.
This to tell Hibernate to fail the validation in the case a user tries to create or update this column to Null.

I also have a comparator for that Entity.
This comparator uses the name field to compare the Entity ( this is just a simplified version of what I have in the project, of course I don't order a bean based on the string length :) )

package com.marco.test;
import java.util.Comparator;
public class ABeanComparator implements Comparator<ABean> {

        @Override
        public int compare(ABean o1, ABean o2) {
                if (o1.getName().length() > o2.getName().length()) {
                        return 1;
                } else if (o1.getName().length() < o2.getName().length()) {
                        return -1;
                } else {
                        return 0;
                }
        }
}

note that there is no null check on the field name, in my project, Hibernate is already taking care of it.

Now, I have a test that create one empty Entity and it stores it into a TreeSet, and then doees other stuff that we do not really care here.
The beginning of the test is similar to the code below:  

package com.marco.test;
import java.util.SortedSet;
import java.util.TreeSet;
public class SortedTestTest {

        public static void main(String[] args) {

                ABean aBean = new ABean();

                SortedSet<ABean> sortedSet = new TreeSet<ABean>(new ABeanComparator());

                sortedSet.add(aBean);
        }
}

If I run this with java 6, everything is OK.

But, with java 7 I have a NullPointerException.  

Exception in thread "main" java.lang.NullPointerException
        at com.marco.test.ABeanComparator.compare(ABeanComparator.java:9)
        at com.marco.test.ABeanComparator.compare(ABeanComparator.java:1)
        at java.util.TreeMap.compare(TreeMap.java:1188)
        at java.util.TreeMap.put(TreeMap.java:531)
        at java.util.TreeSet.add(TreeSet.java:255)
        at com.marco.test.SortedTestTest.main(SortedTestTest.java:14)

Why?

This is why:

    public V put(K key, V value) {
        Entry<K,V> t = root;
        if (t == null) {
            compare(key, key); // type (and possibly null) check

            root = new Entry<>(key, value, null);
            size = 1;
            modCount++;
            return null;
        }

In java 7 when the first Object is added ( if (t == null) ) to a TreeSet, a compare against itself (compare(key,key)) is executed.  

The compare method will then call the comparator (if there is one) and we will have the NullPointerException on the name property. 

    // Little utilities

    /**
     * Compares two keys using the correct comparison method for this TreeMap.
     */
    final int compare(Object k1, Object k2) {
        return comparator==null ? ((Comparable<? super K>)k1).compareTo((K)k2)
            : comparator.compare((K)k1, (K)k2);
    }

This raises more questions than answers:

• Why running a compare if you know that the Object in the TreeSet is the first and only one ?
• My guess is that what they wanted to do was running a simple Null check.
• Why not create a proper null check method ?
• No Answer
• Why wasting CPU and memory running a comparison that is not needed ?
• No Answer
• Why compare an object against itself (compare(key,key))??
• No Answer

This is the put method of the TreeSet in java 6 and as you can see the compare was commented out.

public V put(K key, V value) {
                Entry<K, V> t = root;
                if (t == null) {
                        // TBD:
                        // 5045147: (coll) Adding null to an empty TreeSet should
                        // throw NullPointerException
                        //
                        // compare(key, key); // type check
                        root = new Entry<K, V>(key, value, null);
                        size = 1;
                        modCount++;
                        return null;
                }

You see the comment?  Adding null to an empty TreeSet should throw NullPointerException
So just check if key is null, don't run a useless comparison!


The conclusion? Always try to analyze the code you use, because even in the jdk there is bad code!