rewrite
rewrite copied to clipboard
Recipe: replace object.equals with Objects.equals
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));
}
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.
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
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.
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?
@yeikel an example would be a returned value from a non-null API. In that case, the transformation would not be necessary.
@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
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.
I understand, thank you for explaining.
As a side note, the stream example would need to be refactored to remove the filter
altogether
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
.
As a side note, the stream example would need to be refactored to remove the
filter
altogetherIn this case,
find
may be null, so filteringnonNull
may be important so thatnull == null
does not returntrue
.
That's true. I overlooked it