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!
 

Related Whitepaper:

Bulletproof Java Code: A Practical Strategy for Developing Functional, Reliable, and Secure Java Code

Use Java? If you do, you know that Java software can be used to drive application logic of Web services or Web applications. Perhaps you use it for desktop applications? Or, embedded devices? Whatever your use of Java code, functional errors are the enemy!

To combat this enemy, your team might already perform functional testing. Even so, you're taking significant risks if you have not yet implemented a comprehensive team-wide quality management strategy. Such a strategy alleviates reliability, security, and performance problems to ensure that your code is free of functionality errors.Read this article to learn about this simple four-step strategy that is proven to make Java code more reliable, more secure, and easier to maintain.

Get it Now!  

6 Responses to "Even in the jdk there is bad code"

  1. The compare call is to throw a ClassCastException with the first put().
    Otherwise, with older JVM, we could put an Object, with success, then put an other Object and then have a ClassCastException.

  2. I actually find it quite reasonable when adding stuff into a SORTED
    set to make sure the first added item will be comparable to others. If
    you add an item, whose comparison to any other item (which you might be
    adding later) will throw a NPE, you will kind of block the set, because
    you won’t be able to add any more items then (the ‘add’ method must
    contain a call to ‘compare’ which will fail). Try it with java 6 and add
    two instances of ABean into the set, each of which will have null value
    as the name. It will not work – after inserting one ABean, you cannot
    add any more. So this check in JDK7 prevents you from ‘blocking’ your
    set. By the way, your stuff with @NotNull and nullable=false don’t have any effect here, I don’t see any EntityManager to manage the entities and enforce these rules, you use your ABean as a normal Java object.

    • Marco says:

      The Hibernate annotations have effect in the context of the whole design of the application.
      I don’t put null checks everywhere if I already know that Hibernate will taking care of the mandatory fields (the one in the post is just a simplified version, no need to show the EntityManager there).
      I agree on the fact that if there is something wrong you should stop immediately the process, but using a compare method on a single Object to to this, I still feel is not right.

      • A comparator should be able to compare object A to object A. And if it is not able to, something is wrong, because *most likely* it will not be able to compare object B to object A, therefore not allowing you to add object B to the set later on after you added A. The check might not look very logical at first sight, but in my opinion it is actually quite useful and helps a programmer’s mistake show up earlier…

Leave a Reply


nine × 1 =



Java Code Geeks and all content copyright © 2010-2014, Exelixis Media Ltd | Terms of Use | Privacy Policy
All trademarks and registered trademarks appearing on Java Code Geeks are the property of their respective owners.
Java is a trademark or registered trademark of Oracle Corporation in the United States and other countries.
Java Code Geeks is not connected to Oracle Corporation and is not sponsored by Oracle Corporation.

Sign up for our Newsletter

20,709 insiders are already enjoying weekly updates and complimentary whitepapers! Join them now to gain exclusive access to the latest news in the Java world, as well as insights about Android, Scala, Groovy and other related technologies.

As an extra bonus, by joining you will get our brand new e-books, published by Java Code Geeks and their JCG partners for your reading pleasure! Enter your info and stay on top of things,

  • Fresh trends
  • Cases and examples
  • Research and insights
  • Two complimentary e-books