(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:
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() {
}
}
.
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.
Like this:
Like Loading...
Published by lukaseder
I made jOOQ
View all posts by lukaseder
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,
Objectsis a little less tedious to type or read thanObjectUtils, so one might accept more easily to leave it there in the code.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 withObject.equals(), which could also be contorted as an argument that it shouldn’t have been put onObjectin 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
StringUtilsutility, 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:
But for the sake of argument, let’s review your examples:
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.No one claimed you have to use static import everywhere.
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 callthis.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…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.
The first problem could be alleviated by overriding Object.equals and annotating it with @deprecated (implementation being super.equals).
Good point, although, I think the whole package deserves deprecation in that particular case :)
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 :) :)
toString()is the same story. Overriding is fine. Overloading isn’t a good idea (although widely practiced, even in the JDK).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()andequals()is fine – it’s necessary if you want to put your objects intoHashMaporHashSet. Overloading (as exposed in this article), however, is not a good idea.