Overzealous code analysis enforcement

Development
1

I just got the following error when trying to build some code I just wrote.

[ERROR] Code Analysis Tool has found:
 1 error(s)!
 10 warning(s)
 11 info(s)
[ERROR] org.openhab.core.util.PhoneyScheduledFuture.java:[199]
Use equals() to compare object references.

Here is the code it’s complaining about:

        @Override
        public int compareTo(@Nullable Delayed other) {
            if (this == other) {
                return 0;
            }
            if (other == null) {
                return -1;
            }
            long diff = 0L - other.getDelay(TimeUnit.NANOSECONDS);
            return (diff < 0) ? -1 : (diff > 0) ? 1 : 0;
        }

Except that I disagree with the rule in the first place, there are some places where it’s very common to use == between objects, implementations of equals() and compareTo. But there are other completely legit situations too. I’ve seen many equals() implementations in the code base that doesn’t take all fields into consideration when determining equals(). If you want to test if you have the same instance, you have no choice but to use ==.

Using equals() is a much slower way of doing the same, if equals() isn’t overridden in the class or a superclass, because the implementation of equals() found in Object is:

    public boolean equals(Object obj) {
        return (this == obj);
    }

But, as soon as it’s overridden, you can’t use equals() to ensure that you have the same instance. Often, you can’t know if equals() could be potentially overridden by subclasses.

So, my question is, how do we override such “rules” when they are detrimental to writing correct code?

2

I found openhab-core/tools/static-code-analysis/pmd/suppressions.properties where you can add CompareObjectsWithEquals for the class in question to make this error go away. This was suppressed for a few other classes as well, but I must say that I’m surprised how few. My guess is that the rule has prevented other legit use, or there would be more places that did this in such a large code base.

I get that the rule is there to prevent “rookie mistakes” where people use == when they should use equals(). There are plenty of other ways to achieve this than to outright ban it though, and the idea that it can be “done through other means” just isn’t true. equals() does not evaluate instances, it evaluates whatever algorithm that equals() define.