[EC75] [Java] False positives

[cyChop]

Describe the bug
The Java rule is triggered even when the scope of the String is limited to one iteration.

This rule should be triggered only when the String is declared outside the loop and appended upon inside the loop.

To Reproduce

for (T item : items) {
    if (condition(item)) {
        logger.log("something: " + item.getAttribute()); // Triggers the issue
    }
}

Expected behavior
If the scope of the String is limited to one iteration (object created inside the loop), this rule shouldn't apply.

Screenshots
Need too much anonymization, pseudo-code sample above will be more explicit.

Software Versions

• SonarQube Version: 9.9.1
• Plugin Version: unknown

[diyfr]

Another example

for (T item : items) {
    if (condition(item)) {
        throw new RuntimeException("something: " + item.getAttribute()); // Issue here "Don't concatenate Strings in loop, use StringBuilder instead."
    }
}

[diyfr]

Basic rule exist 1643

Why is this an issue?
Strings are immutable objects, so concatenation doesn’t simply add the new String to the end of the existing string. Instead, in each loop iteration, the first String is converted to an intermediate object type, the second string is appended, and then the intermediate object is converted back to a String. Further, performance of these intermediate operations degrades as the String gets longer. Therefore, the use of StringBuilder is preferred.
Noncompliant code example

String str = "";
for (int i = 0; i < arrayOfStrings.length ; ++i) {
  str = str + arrayOfStrings[i];
}

Compliant solution

StringBuilder bld = new StringBuilder();
  for (int i = 0; i < arrayOfStrings.length; ++i) {
    bld.append(arrayOfStrings[i]);
  }
  String str = bld.toString();```

[cyChop]

@dedece35 said:

Regarding EC75, sorry again, I don't agree. For me it has been a good practice since a long time. Maybe now, with JVM upgrades, this rule could be deprecated. If it is, please could you give us documentation on this point, @cvgaviao, please ?

Several topics here:

The reason behind this issue

Valid case

String myString = "";
for (Item item : items) {
    myString += item.getName();
}

Here, we build a String over several iterations.

Behind the curtains, Java will create a new StringBuilder for each concatenation statement (basically in this example, one StringBuilder per loop).

So yes, the rule applies, because by declaring it themselves, the developer tells the compiler "OK, I'll need it from here to there, so you use this one instead of creating and throwing hundreds of StringBuilders."

Invalid case (false positive)

for (Item item : items) {
    log.info("This is item: " + item.getName());
}

Here, the String lives inside the iteration. It is ready for garbage collection at the end of the iteration, it does not carry to the next one.

Java will use a StringBuilder behind the curtains, but there is no gain for the developer to declare it themselves. It'll only make the work less legible and harder to maintain.

Discussable case

There is one case where the use of StringBuilder could apply, even outside of loops.
I share it here to shed some light on the valid/invalid cases above, but this should not be handled in rule EC75. A new rule would have to be created if we deem this interesting.

String myString = "Name: " + item.getName();
myString += "\nColor: " + item.getColor();
myString += "\nPrice: " + item.getPrice();

The JVM will translate this as follows (there might be optimizations I'm not aware of, I only know the basics):

String myString = new StringBuilder().append("Name: ").append(item.getName()).toString();
myString = new StringBuilder(myString).append("\nColor: ").append(item.getColor()).toString();
myString = new StringBuilder(myString).append("\nPrice: ").append(item.getPrice()).toString();

So we basically created 3 StringBuilder for a toString. In such cases, declaring it explicitly would imply resource savings.

I'm curious about performances and resource usages of StringBuilder vs String.formatted() in Java 17, but that would require a specific analysis.

Reasons for including a rule in ecoCode

For me it has been a good practice since a long time.

ecoCode is not about best practices. SonarQube, Findbugs and so on are dedicated to that.

Our focus is about resource usage, energy gains, and I feel we must really focus on that. It's already years of work for all the contributors. We need to focus on that, especially for rules that natively exist in SonarQube (diyfr shared the link to the rule).

Making false positives an even greater problem

Because EC75 already exists in SonarQube, we're making duplicate warnings each time (EC75 + 1643). False positives are even worse because they're rendered even more visible: "Hey, how come we have a warning from ecoCode but not Sonar, here? How, ecoCode makes false positives on that rule. Well, since it's already in SonarQube, I'll deactivate EC75."

Once again, this is what happened when going live with ecoCode at my client's and getting hundreds of false positives on our codebase.

[dedece35]

Hi @cyChop and @diyfr
EC75 is going to be deprecated : check discussion inside #246 and PR #259

[dedece35]

I close this issue because EC75 deprecated ... please see above