[NEW PLUGIN] ecoCode - Requesting inclusion in SonarQube Marketplace

SonarQube Server / Community Build Plugin Development
4

Hi,

I’ve started testing with the Java version of the plugin, using the project you provided (as communicated via GH since I was already commenting in your project).

Thanks very much for providing a project that seems to raise at least one issue on each rule. TBH, I don’t usually give rule-by-rule feedback, but it’s a small set and I was curious to see your rules in action.

Here’s what I saw in my testing.

To Do

IMO these things should be handled on your side, but if you don’t want to, we need to talk about it.

  • For (cnumr-java:S63), the issue message doesn’t match up with the rule title/description:
    Selection_875
    Selection_876
    While I don’t disagree that what you’ve highlighted is bad practice and should be avoided, having an issue and message that don’t match up to the rule will be confusing for end users.

  • Some of your rules seem to have Snnn ids. While I don’t think there will be a direct clash with our own Snnn rule ids, this still makes me uneasy. Would you consider changing those rule numbers to maybe add a character to the prefix? Or a suffix? Or… something?

  • Correct the plugin key. Yes, I know you know this already. I’m including it here so I have one place to go back to with all the things to double-check.

Optional

  • You might want to shorten, and perhaps translate your rule repo name:
    Selection_874

  • You may also want to re-proof-read your rules (emphasis mine)

…never get it during iteration, It saves CPU cycles so unless energy consumption.

  • After looking at only a few of your rules, it strikes me that most (all?) of them are really about performance. I suggest you also tag them with that: performance

  • At least one Noncompliant Code Example (cnumr-javaS78) doesn’t include a //Noncompliant comment. Without it I, as a user, spend a lot of time going back and forth between the two code samples to figure out what changes & why.

  • Also on S78, I find the wording slightly odd: “in batch update” vs… “in a loop”?

  • S69

Do not call a function when declaring a for-type loop in order to avoid function calls each iterations. It saves CPU cycles.

Might want to sharpen this language a bit. Theoretically it’s fine to call a method in the initialization (int i=foo.size()), right?

  • GRSP0028 - there’s really not enough to go on here. In fact, there’s nothing to go on here. This needs a description.

*GSCIL is a subset of S69. In general you only want one rule to raise an issue on a given piece of code. I suggest dropping this rule.

  • S79, about using try-with-resources raises an issue on code where, at a glance, it looks like all the resources are properly disposed. Users will perceive this as a false positive.

Observation

  • You haven’t provided a default profile. That’s totally fine since you’re not handling the basic language implementation. And I wonder if it makes sense to create a profile that’s all of your rules and all of the built-in #performance rules…?
    My thinking here is not that people would necessarily use your profile as-is, but that perhaps they would copy it into their own profiles & thus ensure that all #performance rules got turned on (and hopefully followed!). This is mostly an idle thought on my side, so feel free to completely ignore it.

 
Ann