Thou Shalt Not Name Thy Method “Equals”

(unless you really override Object.equals(), of course).

I’ve stumbled upon a rather curious Stack Overflow question by user Frank:

Why does Java’s Area#equals method not override Object#equals?

Interestingly, there is a Area.equals(Area) method which really takes an Area argument, instead of a Object argument as declared in Object.equals(). This leads to rather nasty behaviour, as discovered by Frank:

@org.junit.Test
public void testEquals() {
    java.awt.geom.Area a = new java.awt.geom.Area();
    java.awt.geom.Area b = new java.awt.geom.Area();
    assertTrue(a.equals(b)); // -> true

    java.lang.Object o = b;
    assertTrue(a.equals(o)); // -> false
}

Technically, it is correct for AWT’s Area to have been implemented this way (as hashCode() isn’t implemented either), but the way Java resolves methods, and the way programmers digest code that has been written like the above code, it is really a terrible idea to overload the equals method.

No static equals, either

These rules also hold true for static equals() methods, such as for instance Apache Commons Lang‘s

ObjectUtils.equals(Object o1, Object o2)

The confusion here arises by the fact that you cannot static-import this equals method:

import static org.apache.commons.lang.ObjectUtils.equals;

When you now type the following:

equals(obj1, obj2);

You will get a compiler error:

The method equals(Object) in the type Object is not applicable for the arguments (…, …)

The reason for this is that methods that are in the scope of the current class and its super types will always shadow anything that you import this way. The following doesn’t work either:

import static org.apache.commons.lang.ObjectUtils.defaultIfNull;

public class Test {
  void test() {
    defaultIfNull(null, null);
    // ^^ compilation error here
  }

  void defaultIfNull() {
  }
}

Details in this Stack Overflow question.

Conclusion

The conclusion is simple. never overload any of the methods declared in Object (overriding is fine, of course). This includes:

  • clone()
  • equals()
  • finalize()
  • getClass()
  • hashCode()
  • notify()
  • notifyAll()
  • toString()
  • wait()

Of course, it would be great if those methods weren’t declared in Object in the first place, but that ship has sailed 20 years ago.

15 thoughts on “Thou Shalt Not Name Thy Method “Equals”

  1. Java 7 defines java.lang.Objects.equals which is similar to Apache’s ObjectUtils. It has the same problems in regard to static import. However, it’s standard api.

    • Yes, I agree. On the flip side, Objects is a little less tedious to type or read than ObjectUtils, so one might accept more easily to leave it there in the code.

  2. I disagree with things like ObjectUtils.equals being a bad idea for that reason.

    Rather, static import is the bad idea in that example. When you use static import, irrespective of how the *compiler* treats the code, *humans* reading your code will make incorrect assumptions about which method is being called. This affects things like code review – the person who is reviewing your code shouldn’t be forced to go and read the imports just in case a given method call is a static import.

    • (I agree the ObjectUtils.equals() example might be bad in this particular example, because that equals method conflicts with Object.equals(), which could also be contorted as an argument that it shouldn’t have been put on Object in the first place, though).

      If this isn’t about the particular example, then you might be a bit dogmatic :). … and dogma generates more friction among team members than the odd static import. Note that static imports are done to help *humans* read code, not *compilers*. Compilers couldn’t care less if you import anything. What do you find easier to read?

      Your version, if you’re consistent with your argument (never having to go and look up imports):

      if (org.apache.commons.lang.StringUtils.isNotBlank(string)) {
          string = org.apache.commons.lang.StringUtils.trimToEmpty(
              org.apache.commons.lang.StringUtils.leftPad(string, "abc", 10)
          );
      }
      

      … or your version (more probable)

      if (StringUtils.isNotBlank(string)) {
          string = StringUtils.trimToEmpty(StringUtils.leftPad(string, "abc", 10));
      }
      

      … or simply

      if (isNotBlank(string)) {
          string = trimToEmpty(leftPad(string, "abc", 10));
      }
      

      In other words, where’s the added value in explicitly qualifying the methods with the StringUtils utility, here?

      • In a code review, I have to go out of my way to view any code which is outside of a few lines around a diff.

        When I see someone do something like static importing isNotBlank, now I have to go and check whether it’s that notorious one which doesn’t check for null. While doing this, I wonder why the author feels the need to hide the code’s intent.

        That is about the least evil example you could provide, though. Even less evil might be the use of assertEquals in a unit test. That’s fine. Test code isn’t shipped anyway…

        For more examples:
        – there was an isBlank method which did not consider all Unicode spaces and thus should never be used, our own replacement being a uniquely named class which doesn’t involve looking at the imports

        – if you static import ImmutableList.of or ImmutableSet.of them you can’t even tell whether a list or a set is being used

        – if you have an instance method called exit then you have to check the static imports to be sure the coder isn’t hiding a System.exit

        Actually, I would say that if you permit any static imports at all, then *every* call to an instance method looks suspicious and causes me to spend extra time reviewing. 😦

        • You’re certainly either:

          • exaggerating
          • working on a horrible code base and try to deduce generally applicable rules from it
          • overly dogmatic (as I stated before)

          But for the sake of argument, let’s review your examples:

          there was an isBlank method which did not consider all Unicode spaces and thus should never be used, our own replacement being a uniquely named class which doesn’t involve looking at the imports

          Just deprecate and/or remove the particular method. There’s no difference in your work between isBlank(abc), something.isBlank(abc), something.extremely.descriptive.isBlank(abc). I.e. in any case, you have to know that the method is flawed, and once you know you fix the method, not yell at every coder that uses it.

          if you static import ImmutableList.of or ImmutableSet.of them you can’t even tell whether a list or a set is being used

          No one claimed you have to use static import everywhere.

          if you have an instance method called exit then you have to check the static imports to be sure the coder isn’t hiding a System.exit

          Static imports cannot hide instance methods. That’s what this whole article is about. But if you’re so afraid, why not enforce a rule to prefix all method calls with this.? Make developers call this.exit(...), and you’re safe, too. On second thought, given how this discussion evolves, I’m guessing you’re already doing that during your reviews…

          Actually, I would say that if you permit any static imports at all, then *every* call to an instance method looks suspicious and causes me to spend extra time reviewing

          Then, I suggest getting a better job. 😉

          Cheers,
          Lukas

          • You can’t delete a method which is in a library like Commons Lang. You can’t remove the library even if you want to, because in a team of over 10 people, someone will add new calls to it while you’re busy removing others (or worse, you’ll find something with no replacement.) You can’t fix the method because they refuse to accept “fixes” which “change functionality” and other libraries will have made calls to it assuming the broken behaviour.

            The comment about of() was just a “specific example”, since you seemed to be so fond of them. I was illustrating that for any single example where it looks like a good idea to use static imports even if you hate them, there is an example that would be a dumb idea even if you liked them.

            What I meant by the exit example was really that any bare method call forces me to look up the imports. Usually after wasting time trying to find the method in the usual scopes you would expect (the instance methods of the class).

            Yes, a better job (away from Java, ideally) sounds like a good idea. It sounds a lot like you have never had to review somebody else’s code in your life, so maybe wherever you work is really great for freedom or whatever the opposite of code quality is.

            • My friend, I’ve seen code worse than your worst nightmares. But you’re right indeed. I have not yet lived through the luxury of having time to spend on polishing code-bases until they’re all shiny. But perhaps, I’m just too expensive for my customers, so they put me on the real challenges…? 😉

              Cheers, mate. Safe travels through the interwebs.

  3. I once read a book, while I was trying to perfect my skills in Java. It told me exactly how to override hashCode() and other methods in Object, if I really wanted to override Object.equals(). Does this stand still?
    Besides, who will override anything from Object anyway?

    • Ok – I mean besides from toString(), because this is a very convenient way of making debug-string representations of Objects – but I see, this is debatable. A ToString interface could be an alternative, but then again, we all have our ways of StringBuilding the toString() result. Especially when it comes to tree-structures 🙂 Level your debugging 🙂 🙂

    • Ah – who is ever going to do any kind of import, in order to override Object.equals()?

      You guys are way off.

      Don’t embed external logic in Objects that can act perfectly alone.
      Why would you ever do such a thing?

      I have been around for a while, and such approaches just tend to be a mess of libraries, with intermediate dependencies, and you are never quite sure of it all.

      Make a static String.Util.Comparator() or whatever, and then stipulate the use of it!

      It is much, much easier to locate, where this particular static/singleton is being referenced, and all the policies will be centralized,

      And you will ONLYy be using that instance in the business-processing code – not in any Object.equals() implementation !!!!
      Argh!

      So much for equals()!

    • Yes, overriding hashCode() and equals() is fine – it’s necessary if you want to put your objects into HashMap or HashSet. Overloading (as exposed in this article), however, is not a good idea.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s