On SQ 8.6 (yeah yeah I know) the explanation for rule java:S1121 (assignments within expressions) mentions exceptions: “Assignments in while statement conditions, and assignments enclosed in relational expressions are ignored.” But the example illustrates only the first case (while). It would be nice to add an example of the second exception to the description. And more clarification on the form of the exception.
For instance, in this toy example I typed into an IDE with SonarLint, I got:
Sorry the dark mode doesn’t play too nice with the IDE settings, but there’s a blue squiggle under the = in the second if, but not the first. So SL is making an exception for the first assignment, because it’s directly used in a comparison. But wrapping that assignment in another int op makes it ineligible for the exception.
(Of course, this is a stupid example. The real reason for embedding an assignment within a conditional is if you are building an if-else stack for parsing. Something like:
if (line has form X) {
parse line as an X
} else if (line has form Y) {
parse line as a Y
} else if (….
If checking for form X, Y, etc. involves looking for a substring, you often start looking after the substring to parse the rest of the line. So you would do something like
if ((pos = line.indexOf(‘foo’)) >= 0) {
# use pos in finishing the parsing of line
We don’t nag you about using a recent version only to be jerks . We do it because the products change across versions and what you’re complaining about in an old version might already have been fixed in a new one.
Can you check S1121 on the rules site and make sure the current form of the rule description still needs changing in your opinion?
Yeah, just wanted to save you the trouble of telling me to upgrade. (Then again, that could be scripted, so…)
Anyway, from the link you sent it looks like the description is pretty much unchanged.
Ideally, whenever a rule has exceptions, each exception listed should have an example. This seems to be followed most of the time. When I looked up this rule in your search, I found some recent discussion about exceptions, so probably the particular exception was added not too long ago and people just forgot to add it to the description.
Ideally, whenever a rule has exceptions, each exception listed should have an example.
I agree that we can add a second example, the current code is not super clear as it illustrates both a relational expression and an assignment in a switch. PR created to update the description.
As a reminder, relational expressions are:
==
!=
>
>=
<
<=
We only apply the exception when the direct parent is a relational one. In this case, the direct parent is % (arithmetic expression, not a relational one) it explains why the exception does not apply.
That being said, I agree that the decision sounds a bit arbitrary… Maybe we should rethink the exception? One solution could be to consider any assignment in a parenthesis expression as compliant?
We plan to review our rules in the mid-term, to hopefully improve such inconsistencies, this one is definitely on the list.
Yeah, I guessed that’s how the exception worked and why the second case was tagged. But that’s why it should be clearly documented.
Actually, the way you structured the exceptions is pretty good as it is – I’m not necessarily saying that my second case (wrapping the assignment in a non-relational expression) is something I want an exception for.
Of course, most of these code smells are all about readability, which is in the eye of the beholder. So for this particular rule, we added a note to the description saying, in effect: you can violate this rule, but you’d better have a good reason – and be prepared to defend it!
If you change how exceptions are decided, there should be two guiding principles:
1 (and most important): be sure you always catch the “= instead of ==” mistakes. (Who among us hasn’t done that at least a few times? ) Java makes it easy since it doesn’t cast int to bool, so in my two cases at the top, if I had meant to use “j == i…” that wouldn’t have been valid Java because you can’t compare a bool with an int! Might be harder for languages like C though.
2: The exceptions should ideally be only for cases where doing it another way would be clumsy, like a while loop test (which is why you explicitly made that exception). But in my code examples, there really isn’t a valid reason for not requiring the assignment to j to be outside the if. OTOH, if the second if were actually an elseif tied to the first if, then to separate out the assignment would require having an else clause. And if you have ANOTHER elseif clause then you’d need NESTED elses…
Thanks for coming back and sharing more details on this topic, it is interesting to see your point of view on the rule, it will help us to decide if we need to take any other action of not.
YW. Keep in mind that for the ones that are mainly about readability, sometimes there are no “right” answers. Sometimes there are multiple mutually-contradictory SQ rules, like where to put curly braces, so just pick one and stick with it because it’s pretty clear. Other times, sometimes it’s better one way, sometimes better the other way, and you’re not going to get an algorithm to sort it out. Our devs sometimes get into “well THIS way is more readable” arguments – I just look on the bright side: at least they’re THINKING about readability and not just blowing it off!
. We do it because the products change across versions and what you’re complaining about in an old version might already have been fixed in a new one.
(Then again, that could be scripted, so…)