Sonarqube recommends bad refactorings

[banane...@gmail.com]

Sonarqube recommends refactorings for every issue, which are generally okay. However, since Sonar doesn't really know any context of the locally analyzed project, these recommendations might be rather bad in the end.

Simple example: "Mutable members should not be stored or returned directly" (squid:S2384) recommends to create a copy of members. Since we enabled that check, I have to reject a lot of code reviews. We are working a lot with the java collections framework, and using Collections.unmodifiableSet/List/Map() is often a much better fix, since it introduces additional restrictions (by not spreading mutable members, but by actually removing the mutability for clients). I've talked to our developers, I've send mails, but they have forgotten that a week later, when they are suddenly greated by this Sonar finding.

I know perfectly well that I can edit issue descriptions. But my additions are only shown 2 pages down the screen, and no one ever notices them. I think you need to make it possible to either really edit the issue description as such or to add modified descriptions on top instead of at the bottom.

I'm currently considering to disable this and several other checks. While the findings as such are valid, our developers blindly follow the given recommendations and that leads to new issues.

You should also look at this from the viewpoint of framework usage. E.g. in our projects we prefer AssertJ over plain JUnit, Google Preconditions over assert statements or conditional expressions, etc. Since Sonarqube cannot predict the existence of these frameworks, it needs to be more flexible with the recommendations it gives.

Ciao, Michael

[Michel Pawlak]

Hi,

Same problem here. It sounds like another use case for the "adapters" I was thinking about this morning, but instead of modifying the rule's logic, it would modify the recommendation it displays. 

Michel

[G. Ann Campbell]

Hi Michael,

I've entered https://jira.sonarsource.com/browse/SONARJAVA-1672 to address your specific compliant. 

Regarding the broader question of what remediations we suggest, I can only say that we do the best we can. In general, we try to be clear and specific about how to address the problem because we want to be as helpful as possible to the person looking at the issue. When the technologies and frameworks in use change, we try to be as responsive as possible about updating our rules accordingly, but it's hard to stay on top of everything. If there are other specific cases that are bothering you, we'd love to hear about them because you're probably not the only one.

Ann

[banane...@gmail.com]

Hi Ann,

thanks for your reply. It actually underlines parts of the problem. I don't want you to try to "stay on top" of everything and have every refactoring perfect for every user.

Look at it this way: For the scanning part of the Sonar workflow, it seems rather natural that everyone can create his/her own quality profile. And in our company we regulary disable issue types, since they find more false positives than actual issues _in our code_ (while they may be perfect for other users). However, for the second part of the Sonar workflow, the refactoring and removing of issues, no customization of the tool is possible. But it is needed, since many developers in our company blindly follow the suggested refactorings recommended by the tool (if they knew it better, they would not create the issue in the first place).

And that's why to my mind some feature is needed that either allows an admin to edit each and every rule description (to remove unwanted suggested refactorings) or to add some custom text on top of the description. The current approach of appending a custom description after the issue description just doesn't fit this requirement. If you feel there are better technical approaches in Sonar how to avoid developers creating bad fixes due to the issue descriptions, I'm all ears.

Thanks, Michael