[Java:S3973] Ignore, if indentation is "wrong" due to commented code

Clean Code Report False-positive / False-negative...
1 
    /*if (a)
        foo();
    else */if (b) // FP
        bar();
    /*else */if (c) // FP
        xyz();

In the above example rule S3973 reports wrong indentation, which is just because of the commented “else” before it.

I think, this should not be trated like whitespaces and hence not as indentation. At best the indentation check should include the non whitespace part of the comment to also reflect coments like this.

    if (a)
        foo();
/*    else */if (b) // FP
        bar();
2

Hey there.

This rule raises Code Smells, which are issues that impact the readability (and thus maintainability) of code. This commented code should probably be removed for the same reason as S125: Sections of code should not be commented out.

Programmers should not comment out code as it bloats programs and reduces readability.

Unused code should be deleted and can be retrieved from source control history if required.

I think the code sample you shared is definitely less readable with the commented code, which is the spirit of raising an issue here. WDYT?

1 Like
3

I commented about S125 already. And you (the team) agreed, that it should not be enabled by default, because comented code has an important documentary character.

//doItTheOldWay();
doItTheNewWay();

This code example shows commented code, where the reader can easily see, how it was done before and how it is done now. The example in the initial posting is similar. The commented code documents something. When the purpose of the commented code is over, the code will be removed. But not earlier.

Hence it is the same reason here not to raise an issue. Readability is reduced. Yes. But for a reason. And the reason is historical documentation for a time. And for this time there should not be an issue all the time.

5

Readability is reduced. Yes. But for a reason.

When the rule is technically right but you strongly think this is acceptable given the context, this sounds like is a good candidate for marking this issue as “won’t fix” and explaining in the comment the reason.

Now, should this reason be an exception to the rule? I’m not so sure, I would personally be glad to have this code reported. And if I have a good reason to keep it, the issue will help me to track this code and make sure I eventually remove it someday.

7

I think, the rule is wrong in most of the cases. It would have to at least analyze the version control history to see, how long this code is commented out. And after a certain amount of time, I might be happy to get reported about it.

So, this would be a Qube only rule.

It might just be my personal taste. But there’s no (good) candidate for marking an issue in SonarQube. I don’t want any non-code-driven resolution. Do I have control over what happens with the resolution after I delete the marked seciton of code? I don’t. Will the DB grow with dead entries due to this? I have no idea.

I am really not conviced, that this part of Qube should even be there. Just my 1 cent, of course.

8

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.