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!
 

Do you want to know how to develop your skillset to become a Java Rockstar?

Subscribe to our newsletter to start Rocking right now!

To get you started we give you two of our best selling eBooks for FREE!

JPA Mini Book

Learn how to leverage the power of JPA in order to create robust and flexible Java applications. With this Mini Book, you will get introduced to JPA and smoothly transition to more advanced concepts.

JVM Troubleshooting Guide

The Java virtual machine is really the foundation of any Java EE platform. Learn how to master it with this advanced guide!

Given email address is already subscribed, thank you!
Oops. Something went wrong. Please try again later.
Please provide a valid email address.
Thank you, your sign-up request was successful! Please check your e-mail inbox.
Please complete the CAPTCHA.
Please fill in the required fields.

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


6 − = four



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.
Do you want to know how to develop your skillset and become a ...
Java Rockstar?

Subscribe to our newsletter to start Rocking right now!

To get you started we give you two of our best selling eBooks for FREE!

Get ready to Rock!
You can download the complementary eBooks using the links below:
Close