StringBuilder vs string.concat

Question

I was reading this article about StringBuilder vs string.concat and couldn't figure out which one would be better in the following situation:

StringBuilder address = new StringBuilder();
address.Append(retailer.Street.Replace(",", "<br>").Replace("\n", "<br>"));
address.Append("<br>");
if (!string.IsNullOrWhiteSpace(retailer.City)) 
{
 address.Append(retailer.City);
 address.Append("<br>");
}
if (!string.IsNullOrWhiteSpace(retailer.County)) 
{
 address.Append(retailer.County);
 address.Append("<br>");
}
address.Append(retailer.Postcode);
return MvcHtmlString.Create(address.ToString());

According to the article, as it is not in a loop, I should be using string.Concat. But as there are if statements while building the string, I was wondering if that still applied.

Or would I be better off doing the following:

string address = string.Concat(retailer.Street.Replace(",", "<br>").Replace("\n", "<br>"), "<br>");
if (!string.IsNullOrWhiteSpace(retailer.City))
{
 address += string.Concat(retailer.City, "<br>");
}
if (!string.IsNullOrWhiteSpace(retailer.County))
{
 address += string.Concat(retailer.County, "<br>");
}
address += retailer.Postcode;
return MvcHtmlString.Create(address);

Answer 1

DISCLAIMER: You haven't properly defined "better" so I'll assume you're talking about performance

You're fixing the wrong problem.

Under the assumption that your retailer is an instance of some Retailer POCO, you should properly normalize your data and extract the Address data into a separate Table / POCO.

Then you can stop generating HTML in some Controller code and instead do something like:

retailer.Address.ToHTMLString();

This is significantly cleaner and allows central modification of "ToHTML" rules.

In addition to that I don't think you actually have to care about the performance difference between these two. Have you profiled your code and proved that this is the bottleneck? If it isn't.... It's good enough I'd say.
You seem to be falling prey to the problems described in Eric Lippert's Performance Rant

Answer 2

Vogel612 has already covered the performance aspect. I'd like to make a suggestion that will definitely be slower but has an elegance to it:

var addressParts = new[] {
 retailer.Street.Replace(",", "<br>").Replace("\n", "<br>"),
 retailer.City,
 retailer.County,
 retailer.Postcode
};
var nonEmptyAddressParts = addressParts.Where(p => !string.IsNullOrEmpty(p));
var htmlAddress = string.Join("<br>", nonEmptyAddressParts);

You then sidestep your entire Concat vs StringBuilder dilemma.

• Pete: hmmm, nice. But would this not put extra brs in if city and county where empty?
• Vogel612: @Pete those are not included through the Where clause, so no
• RobH: @Pete - I modified the code a bit to make it clearer.
• Pete: Ah sorry, monday! didn't see the linq where bit!

Answer 3

@Vogel612 explained how best to code your solution, but to answer your question about which of the two solution to use, use StringBuilder.

The goal is to request and fill the lease amount of memory possible. When you use String.Concat, the values provided are copied into a new String, so each call requests more memory and fills it. StringBuilder uses a buffer. If the buffer is too small, a new buffer (which, by default, is twice as big as the previous buffer) is created, and the contents of the previous buffer are copied over. To use the string, you must convert it by calling StringBuilder.ToString() which requests the appropriate amount of space and fills in the values. String.Concat does not save you anything over the normal string + string syntax unless you are using one of the overloads which takes more than 2 arguments. In general, do not use String.Concat, use + instead. The .NET compiler will optimize the chained string + string + string ... calls into one concatenate operation, but it cannot optimize the method calls.

In your String.Concat example, you are creating multiple strings using String.Concat and String.Replace, but all of that could be done in one StringBuffer. (Hence, use StringBuffer).

If you want a simple guideline: I personally always use StringBuilder unless I can combine the concatenation into one line of string + string + string ... (because the .NET compiler will optimize these so only one new string is built as mentioned above). a +="A"in a loop is an example when you cannot combine the operation into one line, so a StringBuilder should be used.

The important thing to remember with StringBuilder is that you have to copy the data at least twice, once into the StringBuilder and once when converting the content to a string using StringBuilder.ToString(). This means that (in most cases) you only need to do two string operations to get the same performance using StringBuilder as another option, and additional operations will perform better. The exception to this is if the buffer is too small. You can set the initial size of the StringBuilder buffer by passing it to the constructor. I personally always set the buffer size if I can calculate the max size at construction time, but I know a number of different groups which claim it is a best practice to let the StringBuilder grow as needed. If you know that the StringBuilder's initial buffer size will be too small (it is implementation specific, but I think is is somewhere in the 128 - 1024 element range) then initializing to at least the minimum size and no larger than the max size is probably a good idea, but it will probably not be noticeably different from a performance perspective.

Side note: While it seems possible to optimize the call to StringBuilder.ToString() at compile time to use the existing array as a string (returning any unneeded space), I have not seen anything indicating that this has been done. Considering the amount of work required, the number of rules which would probably be broken, and the limited value, I do not expect it to be implemented.

This is a little longer and more complicated then I expected, so leave a comment if something is not clear.