Sonar and too many "violations"

[bmaehr (Bernhard)]

Hello,

We had some discussions in the team this week and the main reason for it are the too distracting (default) rules.
I have always seen sonar as a great tool for reporting smells in my code. But the (default) ruleset of sonar has moved away from this target and is annoying the developer with suspicious “errors”.

For example the rule squid:S1155:

    if (params.size() == 0) {

The rule suggest to replace .size() == 0 with .isEmpty(). Ok, this is an other possibility to check it, but that doesn’t mean, that one of the possibilities is wrong or a “smell”, it is just an other style. And if in the next line .get(0) is called then even .size() >= 1 seams more logical to me than .isEmpty().

Other examples are the (new) rules squid:S4529, squid:S4834, squid:S4784, squid:S4823, …
These rules report an error because the developer is using HTTP endpoints, regular expressions, permission checks, command line arguments and so on. WTF? Of course these are security-sensitive and the lines should be implemented correctly but they are not an error or smell. In every single line of code there could be perhaps an error, so sonar should perhaps report every line of code as critical.

The last example (the one the main part of the discussion was about) is the rule squid:S3776 about cognitive complexity of code in one method. The rule says by default: If the method has a cognitive complexity of above 15, then is smells. I have to strictly deny this.
Of course it is no good idea to write to complex methods and most of the methods in a project will below 10. But that doesn’t mean, that a value higher than 15 makes the code badly readable and badly maintainable and makes the code smelly or an “error”. I think a usual programmer is easily able to understand (otherwise good written) methods with a cognitive complexity of up to 50. If you take a look at important open source frameworks you will find lots of methods with this complexity. Only “programming monkeys” have perhaps problems with that. Of course it is a good idea to split complex methods into smaller ones if it is possible and makes the code cleaner, but this is true for methods with a complexity of 5 and also for methods with a complexity of 35.
And additionally it would be a good idea to have a possibility to find methods with a high complexity with sonar, because there will be many cases to refactor the code into smaller parts. But it is NOT a smell or dept just because of the complexity (yes, I know there are possibilities to turn the rule of at some line of code or some profile).

So my suggestion for sonar to handle these cases would be to implement a possibility to search for code associated to the mentioned rules, but not treat them as smells or errors.

[Alexandre_Gigleux]

Hello @bmaehr,

First, thank you for coming back to us with a very direct feedback and before answering some of your questions, I will like to remind some basic rules on this forum:

• create on thread by topic because that’s easier to exchange and not mix topics in replies
• stay polite, words can hurt

I understand that your general feedback is: rules of “Sonar Way” quality profile are too noisy for me. You perfectly know that it’s possible to create your own Quality Profile with your own set of rules, I will not go on that field but it’s worth mentioning for the others that are new to SonarQube and will read your post.

Each time we are about to click on the button to say that a rule should be part of “Sonar Way”, we are wondering if the rule would make sense for the majority of the developers of the language. We are not pushing our vision stupidly, we are making a choice to help majority of the developers to write code that is safer, with less bugs and easier to maintain. We are humans and we can do mistakes and that’s one of the goals of this forum to gather this feedback from the community to adjust our default configuration.

squid:S1155: Collection.isEmpty() should be used to test for emptiness

This rule is Minor and this is a Code Smell. Code Smells are not here to say that something is wrong. There is a better way to write this code that is more readable. It will make your code easier to maintain and in some cases, with better performant according to the documentation of the rule:

The time complexity of any isEmpty() method implementation should be O(1) whereas some implementations of size() can be O(n) .

As a developer I would enjoy having this recommendation to show-up in my SonarLint while I’m coding a “params.size() == 0”.

Do you need to refactor all the code that is using “params.size() == 0” ? No. You should always think about the “Fix the Leak” metaphor and concentrate first on fixing issues raised on your New Code.

About the (new) rules squid:S4529, squid:S4834, squid:S4784, squid:S4823

These new rules are very special. They don’t raised issues and you are right by saying “they are not an error or smell”. They raise Security Hotspots that are here to tell you that you are using security-sensitive piece of code. They must be reviewed by someone that care about security and have the bandwidth to review them. Other issues are directly actionable, Security Hotspots are not. That’s why there is a dedicated workflow attached to them and originally we designed them to be used by Security Auditors. Check the Security Reports tab where they are highlighted and separated from the Vulnerabilities on purpose.
Security Hotspots should not impact your Quality Gate unless you transform some of them into a Vulnerability.
I don’t understand why you are saying they are noisy because as a developer, you are not supposed to even see them unless you search for them. With SQ 7.9 LTS we are going to enhance the message related to Security Hotspots to better explain what they are and what you should do with them.
There is probably a bug somewhere or a UI problem if you are bored by these rules. Please raise a dedicated thread so we can investigate that.

Cognitive Complexity
The rule squid:S3776 is a Code Smell. << There is something smelling not good that you should look at when a method is having a Cognitive Complexity higher that 15 >> : that’s what we are basically saying and nothing more. There is no “error” as you are saying and we are perfectly aligned on that. If when you look at a method, you believe that it is perfectly fine to keep it like this, then close the issue as “Won’t Fix” and you are done.
If your guys don’t believe in this “15” value, it’s a matter of 1 min to create a Custom Quality Profile extending “Sonar Way” where you can tune that value to 50 if you want. So what’s the problem?
Regarding your suggestion to be able to find “Complex Methods” based on their complexity value: why not, please create a dedicated thread so we can study that and see what is possible to do today with the API. Maybe some members of the community already faced that situation and have solution for that.

Regards