Isn't that exactly what InefficientStringBuffering is meant to be?

Possibly. We don't have that rule in the subset we use, probably because we didn't update that selection for some time. There is certainly overlap, not sure if they are exactly the same.

On a separate PR for each rule: Juan asked me to create a PR per batch, a couple of rules together. Not sure of best choice here.

They appear to have the same intention, but there are some differences in their implementation:

For example

• your rule flags wrappedLine.append(str + "c", a, c); correctly whereas the other rule gives up
• your rule flag sb.append(1 + 2), which is not a concatenation, so that's a false positive
• your rule considers concatenation with a non-static final field as a constant expression, which it isn't
• the other rule doesn't flag any cases of new StringBuilder().append(...), ie use of append directly on the constructor invocation, whereas yours does it correctly

I think we should fix the false negatives of the existing rule but not merge this new rule.

Actually this rule has many more problems... It uses too many // tests, so that you end up testing unrelated nodes. Consider this violation where the rule sees an append, a string concatenation somewhere in another statement (the throw statement) and reports on a common ancestor (the for statement), even though the concatenation and the append are unrelated. All the violations I've seen on the spring framework in the report are false positives either of this kind, or where there's no concatenation but just numeric addition.

I agree, we should enhance the existing rule InefficientStringBuffering instead of adding this new rule.

FWIW: I've noticed, that the found violation don't highlight the append - at least for the 3 violations found in checkstyle: I wouldn't know, what to fix, if I get these violations. We should aim to report exactly the node which contains the concatenation. Maybe the reason for this is the //, that Clément mentioned?

The assumption of the rule doesn't hold if the calendar is not created just to call this method, or if it is eg passed as a parameter and not created locally. Consider for example this violation in the Spring Framework, which looks like a false positive.

Actually this rule has many more problems... It uses too many // tests, so that you end up testing unrelated nodes. Consider this violation where the rule sees an append, a string concatenation somewhere in another statement (the throw statement) and reports on a common ancestor (the for statement), even though the concatenation and the append are unrelated. All the violations I've seen on the spring framework in the report are false positives either of this kind, or where there's no concatenation but just numeric addition.

The rule should be checking that the variable being concatenated is defined outside the loop.
Consider this violation:

      for (String fileExtension : fileExtensions) {
				if (fileExtension.charAt(0) != '.') {
					fileExtension = "." + fileExtension; // here
				}
				this.fileExtensions.add(fileExtension);
			}

The concatenation is local to each iteration and replacing it with a StringBuilder would only be noise.

Maybe a better name would be UseIOStreamsWithApacheCommonsFileItem? IMHO "streaming" immediately hints at the Stream API and not IO streams, so I think it would be clearer

Could you mention getInputStream() explicitly in the description? Like for the name of the rule, the "streaming methods" here sounds unclear to me

FYI: Just use markdown syntax for the description. No need to use 
 for a new line, just add a empty line. We'll parse the text here (including the line breaks) and use it in our markdown-based rule documentation.
In order to easily read this on github, also consider breaking the lines after 120 characters.

typo: contact -> concat

The better one:

StringBuilder sb = new StringBuilder("arg='").append(arg).append("'");

Perhaps, it's better to use best practices or at least mention that this example isn't the best.

Agreed, we should change the sample code. Note: performance.md is generated, the actual change has to happen in performance.xml.

Note: performance.md is generated, the actual change has to happen in performance.xml.

Is it something that can be automated with Danger? :)

Is string concatenation is still an issue nowadays (2019) ? I've heard that there were optimizations for that in JDK9, for example.

There is indeed some optimization done with Java 9:

• http://openjdk.java.net/jeps/280
• https://stackoverflow.com/questions/46512888/how-is-string-concatenation-implemented-in-java-9

Before Java9, inside the loop, the code generated was basically:

StringBuilder temp = new StringBuilder();
log = temp.append(log).append(val).toString();

Since Java9, it looks more like (roughly):

log = StringConcatFactory.makeConcatWithConstants(..., log, val);

So, the rule is definitely less relevant for java9+.

it's worth noting, that this specific example would be unaffected by Java 9+ optimizations, as the concat is done within a loop