We’re still on an old SonarQube, so maybe you’ve addressed this already. But I checked over at Java static code analysis | Code Smell: Methods returns should not be invariant which I assume is kept up to date, and I don’t see anything about an exception. So if you’ve addressed this and I just need to upgrade, you still should change your rule description.
Basically, rule java:S1132 says if foo is a string, replace foo.equals("bar") with "bar".equals(foo) to preclude NPE problems. In one issue flagged in our code, we are checking for possible “blank” cases with something like
if (foo == null || foo.trim().equals("")) {...
The NPE checker java:S2259 doesn’t complain about foo.trim(). That rule’s flow checker is able to see that the null check with short circuit logic prevents a method call on a null object. So the question is, why is it getting dinged for S1132?
So a rigid application of S1132 would mean changing the right half to something like "".equals(foo.trim())... Now this is one of those cases that could go either way. Some might argue that you should ALWAYS follow S1132 to get into the habit. But others would argue that the original form is more readable, because it’s parallel construction (foo is first in both clauses, so obviously foo is what you’re interested in).
If you take the latter view, then this application of S1132 is clearly FP. You already have logic to check nulls and filter out cases with proper null-check predicates. So you just need to have S1132 tie into that logic.
BTW when I saw my formatted post, the link to your other page shows “Method returns should not be invariant” – yet the link takes you to the right page (S1132) so something is wrong here?
The second non-compliant example is similar to the example I posted, but simpler (no call to .trim() to complicate things). You might add a note to the effect that even though the second line is properly null-checked, the compliant replacement is still preferable since when you invert it like that, you get the null check “for free”. (For the benefit of people who aren’t Java gurus.)
Note that that url is rules DOT sonarsource DOT com/java/type/Code%20Smell/RSPEC-1132 (need to jump through hoops to keep your system from turning it into a link).
Now if I go to the parent (i.e., take off the RSPEC-1132 part) and then type “left” in the search bar, the third one on the search hit list looks like the right rule. And when I click on it, I get
which looks right. But the URL looks the same! And if I then copy-paste the URL from that new page into a DIFFERENT browser (so not a caching issue), I get something like the original screenshot (where the title says “Strings literals” but the text talks about returning invariant values).
(And shouldn’t that be “String literals” rather than “Strings literals” ? )
