I’d like to invite some discussion of this older post which has since been closed. The gist is that a user posted that they like to have short one-line conditionals (if(!a) { b = c; }) on a single line to improve readability, and the response from the Sonar team was that they find one-line conditionals to be less readable, and there the conversation ended.
This is clearly a matter of personal opinion, on which reasonable people can disagree. I could find a dozen well-respected style guides to argue on both sides of the issue. I’m not here to re-litigate a religious argument, I just want to cut down the noise in my Sonar results. The simple fact is that when this rule showed up, my “medium sized” project flagged over a thousand violations. I’m sure almost all of them are for single-line conditionals (which are acceptable under our style guide) but maybe once or twice, I wrote foo.bar();baz(); instead of foo.bar().baz(); – this rule is a great opportunity to catch errors like that!
Now, I could review those thousand instances to see if one or two are a real error, but that would take ages. That’s why I’m writing this post, asking you to reconsider your stance. I’d like to see either a property on the rule that lets single-line conditionals pass, or I’d like to see the rule split into two, leaving the current one to check for actual multi-statement lines (treating conditions and body-blocks as separate “lines”), and one to check for single-line conditionals, so I could configure my project to ignore the latter. (I think there’s already a rule to flag conditionals that don’t use braces in JS/TS; certainly if there’s not, there should be.)
I think this change is worth pursuing because the rule as written today achieves two goals, which should be treated separately on their merits. On the one hand, it’s a readability issue, but those are relatively low severity and I don’t think Sonar is otherwise too opinionated about style-guide stuff. On the other hand, it can catch errors of the class I described above, which should be treated as fairly severe.
Don’t worry about it. I’m actually relying more on @ts-eslint more than Sonar these days – eslint in real time, Sonar every once in a while when I have time to look at it. FWIW, eslint has a flag for the brace-style rule called allowSingleLine, which I’ve enabled. I’d like to be able to make Sonar behave in the same way.
Re style guides, I found Google C++ Style Guide – the last example under “For historical reasons…”:
if an if statement has no else or else if clauses, then the curly braces for the controlled statement or the line breaks inside the curly braces may be omitted if as a result the entire if statement appears on… a single line
(They also allow for omission of braces, which is of course a terrible idea )
I suspect there will be more guides that dictate always placing the statement on multiple lines, but IMHO that’s because it’s easier to codify one rule that applies in all cases than to say something subjective like “if the conditional and statement are short enough…” which is somewhat subjective.
In fact S1105 is brace-style from eslint with allowSingleLine, and S122 is max-statements-per-line from eslint with no configuration available to disable the rule for if (cond) { foo() }. So I would rely on ESLint community here and suggest you to disable S122 or discuss with ESLint if they can add a configuration to max-statements-per-line. On our side it does not seem to worth re-implementing S122.
Thanks Elena, per your suggestion I’ve asked for an extra flag – I’m not holding my breath, but we’ll see what they say. As I mention in that issue, it’s surprising because they do explicitly already allow single-line conditional statements, but only if they’re not in a block – but I’m required to use a block because I have the curly rule as well. Can’t win either way
)
)