Assertions about comparing different types: redundant and silly?

Question

So I have this Java code that overrides equals for a class:

@Override public boolean equals(Object o) {
    if (o instanceof NumeroSeccion) {
        NumeroSeccion numSec = (NumeroSeccion) o;
        return Arrays.equals(this.valor, numSec.valor);
    } else {
        return false;
    }
}

which basically follows the common rule about checking if both objects are of the same class before doing anything else. Others prefer using getClass(), but the idea is the same.

Now I wrote some tests for it, including:

void notEqualsOtherClass() {
    NumeroSeccion ns1 = new NumeroSeccion("1.2.3.4.5");
    String ns2Str = "1.2.3.4.5";
    assertNotEquals(ns1, ns2Str);
}

According to IntelliJ IDEA, this is "possibly redundant":

Possibly redundant assertion: incompatible types are compared 'NumeroSeccion' and 'String'

According to Sonar static code analysis, this is a "silly equality check":

Silly equality checks should not be made. Comparisons of dissimilar types will always return false. The comparison and all its dependent code can simply be removed. This includes: comparing unrelated classes.

Is it actually redundant checking class types or add a test for this check, or are all these tools giving bad advice?

score 4 · Accepted Answer · answered May 11 '23 at 08:58

The advice given by the warning is valid for production code. Testing code is a special case, where you are deliberately doing something wrong in order to test something. So, the warning is perfectly legitimate, but it just so happens to be inapplicable in testing code.

You can safely suppress the warning in that specific line of test code only.

Alternatively, instead of suppressing the warning you can use a trick to fool the compiler (and/or the IDE) so that the warning is not issued. For example, instead of String ns2Str = "1.2.3.4.5"; you may do Object ns2Str = "1.2.3.4.5"; This will probably get rid of the warning.

Thank you. I tend to forget that assertions are used outside testing code too. And it looks like Sonar's advice is for production, considering the example. The trick worked well and it makes sense. — karmapolice, May 11 '23 at 09:26

score 1 · Answer 2 · answered May 11 '23 at 09:39

1

First, in your implementation of Object::equals, the use of instanceof and the comparison of Object::getClass for this and other are not equivalent!

public class Parent {}
public class Child extends Parent {}

final var v1 = new Parent();
final var v2 = new Child();

final var check1 = v2 instanceof Parent; // is true
final var check2 = v2.getClass().equals( v1.getClass() ); // is false
final var check2 = v2.getClass().equals( Parent.class ); // is false

For the implementation of equals(), this makes a significant difference. Usually, you should use instanceof in implementation of equals() only for final classes.

The warnings you got from your tools are because these know the result of the comparisons already before the execution of your program, and therefore they assume that the respective code is at least obsolete, if not wrong. The tools did not look into your implementation of equals(), but they assume a proper implementation, so they also assume that an instance of NumeroSeccion could never be equal to an instance of String.

answered May 11 '23 at 09:39

tquadrat

3,033
1
16
29

1

I would debate your claim about instanceof. Polymorphism dictates that subclass instances should preserve equality behavior. – VGR May 11 '23 at 15:06
@VGR – With `instanceof ` in `equals()`, `v1.equals( v2 )` can be true, but `v2.equals( v1 )` is always false. And that hurts the requirements for implementations of `equals()`. – tquadrat May 11 '23 at 22:04
Not if v2 inherits the same equals method. – VGR May 11 '23 at 22:47
@VGR – If the children inherits the `equals()` method from the superclass, a child can be equal to a parent. Have fun to explain that in the documentation. – tquadrat May 12 '23 at 01:14
@VGR and tquadrat — Whether a child should be able to be equal its parent depends on the use case and who you're asking. For instance, a `LinkedHashSet` can equal a `HashSet`, since they use the definition of equality mandated by the `Set` interface. And for standard data objects (such as discussed in this answer), there's arguments to be had both ways of whether to allow subclasses to be equal to their parents. See [Any reason to prefer getClass() over instanceof when generating .equals()?](https://stackoverflow.com/q/596462/1108305) for arguments both ways. – M. Justin May 12 '23 at 22:11
@M.Justin – Of course you can define use cases where a child can be equal to a parent, but you still have to make sure that the requirements from https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Object.html#equals(java.lang.Object) are met. And your `Set` sample is weak: although `LinkedHashSet` is in fact a child of `HashSet`, they are semantically siblings – also a `TreeSet` instance can be equal to a `LinkedHashSet` instance (see https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Set.html#equals(java.lang.Object)). – tquadrat May 13 '23 at 01:33

Assertions about comparing different types: redundant and silly?

2 Answers2