Recipe: replace object.equals with Objects.equals

[yeikel]

Using object.equals can lead to NPE. For example :

String a = null;
String b = "";

boolean equal = a.equals(b); // NPE

Objects.equals does not have this problem :

String a = null;
String b = "";

boolean equal = Objects.equals(a,b); // false

See : https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#equals-java.lang.Object-java.lang.Object-

    /**
     * Returns {@code true} if the arguments are equal to each other
     * and {@code false} otherwise.
     * Consequently, if both arguments are {@code null}, {@code true}
     * is returned and if exactly one argument is {@code null}, {@code
     * false} is returned.  Otherwise, equality is determined by using
     * the {@link Object#equals equals} method of the first
     * argument.
     *
     * @param a an object
     * @param b an object to be compared with {@code a} for equality
     * @return {@code true} if the arguments are equal to each other
     * and {@code false} otherwise
     * @see Object#equals(Object)
     */
    public static boolean equals(Object a, Object b) {
        return (a == b) || (a != null && a.equals(b));
    }

[tkvangorder]

I think this recipe would lead to A LOT of changes that are not desired. You would end up with a ton of cases where the left side of the object is known to be non-null.

[yeikel]

I think this recipe would lead to A LOT of changes that are not desired. You would end up with a ton of cases where the left side of the object is known to be non-null.

What do you mean?

The idea of this recipe would be to avoid NPE in all situations and avoid an unnecessary null check

[jkschneider]

The correct answer is somewhere in the middle... The most "do-no-harm" approach initially would be to check if there is a potential null on the left side and replace with Objects.equals. We should be able to do a reasonable job of this with dataflow analysis soon.

[yeikel]

The correct answer is somewhere in the middle... The most "do-no-harm" approach initially would be to check if there is a potential null on the left side and replace with Objects.equals. We should be able to do a reasonable job of this with dataflow analysis soon.

Just for my own understanding, under what conditions would this cause harm?

[traceyyoshima]

@yeikel an example would be a returned value from a non-null API. In that case, the transformation would not be necessary.

[yeikel]

@yeikel an example would be a returned value from a non-null API. In that case, the transformation would not be necessary.

Thank you

Considering what the method does, it won't hurt but I understand your reasoning

[traceyyoshima]

I think there are a lot of scenarios to consider in which the changes wouldn't help code quality, and may introduce tech debt.

These are arbitrary examples, but consider:

boolean method(List<String> strings, String find) {
    return strings.stream()
        .map(o -> o /* so something */)
        .filter(Objects::nonNull)
        .anyMatch(o -> o.equals(find));
}

or

if (object != null) {
   ... object.method();
   if (object.equals(X))
   ...
}

There could be a lot going on in the code; intentional API designs, and/or legacy code that's difficult to untangle, etc.

I think Jon's point is significant since we'll be able to make more informed transformations when data flow is implemented.

[yeikel]

I understand, thank you for explaining.

As a side note, the stream example would need to be refactored to remove the filter altogether

[traceyyoshima]

As a side note, the stream example would need to be refactored to remove the filter altogether

In this case, find may be null, so filtering nonNull may be important so that null == null does not return true.

[yeikel]

As a side note, the stream example would need to be refactored to remove the filter altogether

In this case, find may be null, so filtering nonNull may be important so that null == null does not return true.

That's true. I overlooked it