Why does the equals() implementation generated by Eclipse check for null before type checking (instanceof)?

Question

I regularly used Eclipse's code generation tools (Source / Generate hashCode() and equals()...) to create the equals() implementation for simple POJO classes. If I choose to "Use instanceof to compare types" this produces an equals() implementation similar to this:

  @Override
  public boolean equals(Object obj) {
      if (this == obj) {
          return true;
      }
      if (obj == null) {
          return false;
      }
      if (!(obj instanceof MyClass)) {
          return false;
      }
      MyClass other = (MyClass) obj;
      // check the relevant fields for equality
  }

Today a colleague pointed out, that the second if statement is not necessary at all, since the instanceof type check will return false whenever obj is null. (See question 3328138.)

Now, I guess that the folks writing the code templates for Eclipse JDT are worth their salt, too. So I figure there must be some reason for that null check, but I'm not really sure what it is?

(Also question 7570764 might give a hint: if we use getClass() comparison for type checking instead instanceof, obj.getClass() is not null safe. Maybe the code template is just not clever enough to omit the null check if we use instanceof.)

EDIT: Dragan noticed in his answer, that the instanceof type check is not the default setting in Eclipse, so I edited that out of the question. But that does not change anything.

Also please do not suggest that I should use getClass() or (even better!) a different IDE. That's not the point, that does not answer the question. I didn't ask for advice on how to write an equals() implementation, whether to use instanceof or getClass(), etc.

The question roughly is: is this a minor bug in Eclipse? And if it's not, then why does it qualify as a feature?

Grantley

don't know, but instanceof isn't always considered the best way to go. A different approach is to compare obj.getClass() with this.getClass(), and then you do need the null check – Needlecraft

i would quote a comment on this answer. Specifically, in Item 8, he notes that in equals() methods, one instanceof operator serves two purposes - it verifies that the argument is both non-null and of the correct type. "...[S]o you don't need a separate null check." – Predestinarian

IntelliJ Idea generates an equals implementation that includes the null check, but is using getClass(): if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; /*...*/ – Isomerize

Using eclipse Luna. The generated code getClass() != obj.getClass() after the null check. – Dacosta

Whether to use instanceof or getClass() is covered nicely in Any reason to prefer getClass() over instanceof when generating .equals(). Josh Bloch has favored using instanceof; his arguments can be found from this (old) interview. – Isomerize

Answer 1

It is unnecessary because instanceof has a built in null check. But instanceof is a lot more than a simple foo == null. It is a full instruction preparing a class check doing unnecessary work before the null check is done. (see for more details http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.instanceof)

So a separate null check could be a performance improvement. Did a quick measurement and no surprise foo==null is faster than a nullcheck with instanceof.

But usually you do not have a ton of nulls in an equals() leaving you with a duplicate unnecessary nullcheck most of the times... which will likely eat up any improvement made during null comparisons.

My conclusion: It is unnecessary.

Code used for testing for completeness (remember to use -Djava.compiler=NONE else you will only measure the power of java):

public class InstanceOfTest {
    public static void main(String[] args) {
        Object nullObject = null;

        long start = System.nanoTime();         
        for(int i = Integer.MAX_VALUE; i > 0; i--) {
            if (nullObject instanceof InstanceOfTest) {}
        }
        long timeused = System.nanoTime() - start;  

        long start2 = System.nanoTime();
        for(int i = Integer.MAX_VALUE; i > 0; i--) {
            if (nullObject == null) {}
        }
        long timeused2 = System.nanoTime() - start2;

        System.out.println("instanceof");
        System.out.println(timeused);       
        System.out.println("nullcheck");
        System.out.println(timeused2);
    }
}

Steelwork

I am sure the things the instanceof instruction does before an internal null check are nothing compared to a manual null check, especially in an optimized environment. – Lecky

In an optimized environment I expect a 'manual' nullcheck to be optimized as good as the internal one. (speaking assembler both would come down to a MOV CMP JNE ) – Steelwork

Instanceof as an instruction using the stack has a pop (reference), push (result) more than a simple nullcheck. How much this would use up in reality is hard to tell and likely different based on os and hardware. (thats why profiling is the desired option to check for performance issues) – Steelwork

Answer 2

Indeed, it is unnecessary and it is the mistake of the authors of the Eclipse template. And it is not the first one; I found more of smaller errors there. For example, the generation of the toString() method when I want to omit null values:

public class A {
    private Integer a;
    private Integer b;

    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append("A [");
        if (a != null)
            builder.append("a=").append(a).append(", ");
        if (b != null)
            builder.append("b=").append(b);
        builder.append("]");
        return builder.toString();
    }
}

If a is not null and b is, there will be an extra comma before the closing ].

So, regarding your statement: "Now, I guess that the folks writing the code templates for Eclipse JDT are worth their salt, too.", I assume they are, but it would not hurt them to pay more attention to these tiny inconsistencies. :)

Josi

OK, now I'm not sure what the default is (or was), so I edited that part out of the question. – Grantley