dotnet / roslynator

Roslynator is a set of code analysis tools for C#, powered by Roslyn.
https://josefpihrt.github.io/docs/roslynator
Other
3.07k stars 256 forks source link

Refactor: Append interpolated or concatenated string to Append calls #1562

Open wmjordan opened 1 week ago

wmjordan commented 1 week ago

Given the following code:

var sb = new StringBuilder();
var var1 = DateTime.Now.ToString();

sb.Append($"ABC{var1}EFG");

The third line used string interpolation, which is unnecessary, can be refactored to a series of Append calls as the following code shows.

sb.Append("ABC").Append(var1).Append("EFG");

Moreover, it is also useful to change Append concatenated string to Append calls as well. For instance, the following can also be refactored to the above statement.

sb.Append("ABC" + var1 + "EFG");

// or

sb.Append(String.Concat("ABC", var1, "EFG"));
josefpihrt commented 1 week ago

Following code will use AppendInterpolatedStringHandler under the hood which is even better then StringBuilder.Append methods.

sb.Append($"ABC{var1}EFG");

SharpLab: https://sharplab.io/#v2:CYLg1APgAgTAjAWAFBQAwAIpwHQBUCmAHgC4Dcyys6AwsgN7LpOYAs6AsgBQCUjzDSZkPQA3AIYAndAGcARugC86AHb4A7ugDKxCQEtlAcwBCAV10AbYPgk9yg4U3FSncRegAiY4vly6AtvjYAHIA9mp4Idp6hrYU9g5y2ACCAA4p+MrAnAAkAERJRtR0LgC+AKIAYgDiudx2QiXIjUhAA==

Following code will be converted into previous case by RCS1267.

sb.Append(String.Concat("ABC", var1, "EFG"));

Following code could be added to RCS1267 as an improvement.

sb.Append("ABC" + var1 + "EFG");
wmjordan commented 1 week ago

Yes, I know about the AppendInterpolatedStringHandler thing. However, in my practical situation, there are quite a few Append calls around this one. Thus the StringBuilder can not be simply substituted by a interpolated string. For example,

var sb = new StringBuilder();
var var1 = DateTime.Now.ToString();

for (var i = 0; i = 100; i++) {
    sb.Append(somethingElse(i));
}
sb.Append($"ABC{var1}EFG");

or sometimes, the sb.Append(interpolated string) is invoked within a loop. So, it is arguable that the string interpolation will bring ultimate benefit to the overall performance.

Leaving the AppendInterpolatedStringHandler thing there, will generate a lot of intermediate string instances, cause extra memory copying, and put burden to the GC. If we break it into Append calls, there will be no such burden then.

josefpihrt commented 1 week ago

Thus the StringBuilder can not be simply substituted by a interpolated string.

Not sure if I understand. Why it couldn't be substitued?

sb.Append(interpolated string) is invoked within a loop

Why it matters if it's in a loop or not?

will generate a lot of intermediate string instances, cause extra memory copying

Where do you see these allocations in the SharpLab example?

wmjordan commented 1 week ago

You are right. From .NET 6 on, there is no significant performance degrade when calling StringBuilder.Append(interpolated string).

Sorry, I did not mentioned that the situation was that the code was compiled for the old .NET Framework where no AppendInterpolatedStringHandler exists. I checked the decompiled code from an assembly and the code is compiled as a call to String.Format, then StringBuilder.Append(string).