The rule S00108 states, that nested blocks should not be empty, because this could indicates accidentally missing code. The block should either be filled with code or removed. However, if a comment is added, the block is not considered empty, although the block is still doing nothing.
The rule came up by the following example:
if (conditionIsMet()) {
doSomething();
} else {
// nothing is to be done if the condition is not met
}
The comment shadows an empty block and in my opinion adds nothing of value: If there was no else-branch, one would immediately see, that nothing must me be done if conditionIsMet() == false; if one want’s to indicate that one considered the else-case the comment could be moved to the then-branch.
What’s the reasoning behind this behavior? Would it be possible to make the check for a comment optional?
If I understand you correctly, you consider your code example as false-negative for S00108. You want this rule to report empty blocks even if there is a comment inside.
The idea of this rule is to detect places where some logic is missing. These is an assumption on our side that if developer took care to write a comment, empty block is not a problem for the program.
Also sometimes it’s more readable with empty (I mean without code) block, e.g.:
if (a) {
do1();
} else if (b) {
// do nothing
} else if (c) {
do2();
}
// vs
if (a) {
do1();
} else if (!b && c) {
do2();
}
While in your case indeed empty block could be just removed
if (conditionIsMet()) {
doSomething();
}
I believe it’s out of responsibilities of this rule, as in this case it’s more a detection of sub-optimal code structures, than finding forgotten code blocks.
Hi @Lena,
thank you for your reply. Your example indeed illustrates why the current rule implementation is useful. I agree with you that your first version is easier to read, easier to understand and with a proper comment it should be clear why the empty branch has no implementation.
I fear, however, that it is too easy to subvert the rule’s intention by simply adding a useless comment like //. This might be case which should be detected by code review, though.
Another risk are comments indicating that an implementation is planned after a certain event occurred: // trigger update when service is implemented.
You probably discussed these cases, when the rule was specified?
Since we agree, that an empty else-branch is unnecessary, even with a comment, could the rule be adapted to always report this case?
Would you accept a pull request modifying the rule to a) report else-branches even with comments and/or b) make the check for comments optional?
This rule is pretty straightforward: it finds code block without any statement and considers it compliant if there is a comment. So to fix the issue you can do 2 things: drop code block or put comment explaining why this code block is empty and should not be removed.
And I believe that such comment might be useful even for else-branch.
Also I think making comments optional is not the great idea, there still be cases when you will want to leave comment.
Your previous example demonstrated that it is perfectly valid to have empty blocks and add a comment explaining, why the block is empty. My problem with the rule is, that it can’t distinguish between an empty block with a reasonable comment like
if (a) {
do1();
} else if (b) {
// Sophisticated comment explaining why no action is necessary
} else if (c) {
do2();
}
and an empty block with a useless comment like
if (a) {
do1();
} else if (b) {
// LOL
} else if (c) {
do2();
}
In my opinion the latter example is a false negative and thus a failure of the rule. One could argue, that code review should detect such comments but that shifts the burden to detect problematic code. I’d rather have the Sonar analyzer (which does a wonderful job, by the way) detect both cases and mark the false positive myself. The alternative gives a false sense of security where it appears like all empty blocks have proper commentary.
In our code base empty blocks are the exception and any such block is suspicious. In other code bases this might be different, hence the suggestion to add a parameter to the rule to optionally report code blocks even when a comment is present.
But I fully understand, if such a change is not suitable for all users.
I see your point, you want to be super strict. I’m afraid that when we develop our analyzers we try to follow pretty opposite line: we should have the least possible number of false positive. Also we prefer to add rule configuration only when it’s really required. Here I’m not sure a lot of user will follow you. Anyway that was interesting insight, and of course we might reconsider this decision in future if more feedback like yours received.