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!
 

Reference: Even in the jdk there is bad code from our JCG partner Marco Castigliego at the Remove duplication and fix bad names blog.



Source : http://www.javacodegeeks.com/2013/04/even-in-the-jdk-there-is-bad-code.html

Back to Top