Fix PMD rule LiteralsFirstInComparisons for compareTo* and contentEquals

[punkratz312]

Simply switch the arguments; that should be a very small change to make, just similar to the Inline Variable recipe.

What problem are you trying to solve?

https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#literalsfirstincomparisons

Describe the situation before applying the recipe

class A {
    boolean bar(String x) {
        return x.equals("2"); // should be "2".equals(x)
    }
}

Describe the situation after applying the recipe

class A {
    boolean bar(String x) {
        return "2".equals(x);
    }
}

Further examples from the PMD:

class Foo {
    boolean bar(String x) {
        return x.equals("2"); // should be "2".equals(x)
    }
    boolean bar(String x) {
        return x.equalsIgnoreCase("2"); // should be "2".equalsIgnoreCase(x)
    }
    boolean bar(String x) {
        return (x.compareTo("bar") > 0); // should be: "bar".compareTo(x) < 0
    }
    boolean bar(String x) {
        return (x.compareToIgnoreCase("bar") > 0); // should be: "bar".compareToIgnoreCase(x) < 0
    }
    boolean bar(String x) {
        return x.contentEquals("bar"); // should be "bar".contentEquals(x)
    }
}

[punkratz312]

I would love to volunteer and implement it if you could provide me with the snippet to flip the arguments. It should be very similar to my favorite rule, 1488 InlineVariable. https://docs.openrewrite.org/recipes/staticanalysis/inlinevariable.

[knutwannheden]

For the equals() case there is already a recipe here: https://github.com/openrewrite/rewrite-static-analysis/blob/main/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java. The others are not covered as far as I know.

[timtebeek]

Thanks for the offer to help @punkratz312 ! As Knut has indicated String#equals and String#equalsIgnoreCase are already handled in this existing recipe:

rewrite-static-analysis/src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNullVisitor.java

Lines 32 to 34 in 2163f0f

	public class EqualsAvoidsNullVisitor<P> extends JavaVisitor<P> {
	private static final MethodMatcher STRING_EQUALS = new MethodMatcher("String equals(java.lang.Object)");
	private static final MethodMatcher STRING_EQUALS_IGNORE_CASE = new MethodMatcher("String equalsIgnoreCase(java.lang.String)");

I suppose contentEquals could easily be added there as well, and we'd welcome such a change. Once that's covered a separate recipe can likely do the same for compareTo and compareToIgnoreCase.

Great to see you join the project and actively look for areas to improve. :)

[punkratz312]

Thanks for the quick win! I'm looking forward to this fix in our codebase. When will it be available after the merge?

• PMD: LiteralsFirstInComparisons for compareTo* and contentEquals #365

[timtebeek]

Once merged it'll be available from our snapshot versions in a matter of minutes (provided the build succeeds). From there it'll get included in the next release; which is every other week on Wednesday, with the next one on the 23rd. Thanks again!

[timtebeek]

• Fixed in address: LiteralsFirstInComparisons PMD rule #368. Thanks again!

[timtebeek]

A subtlety was missed in the earlier implementation, where the comparison used with compareTo needs to be inverted, such that < 0 becomes > 0, as pointed out in

• EqualsAvoidsNull wrongly inverts a.compareTo(b) #442