current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite
1

Here's the current code: 

    static string GetResources(string header, string filter, string resourceTemplate)
    {
        string themeName;
        if (!TryGetHeaderValue(header, "theme", out themeName)) return "";

        var resources = "";
        var path = Path.Combine(HtmlPreview.BaseDirectory, themeName);

        foreach (var resource in Directory.GetFiles(path, filter))
        {
            resources += String.Format(
                resourceTemplate,
                themeName,
                Path.GetFileName(resource));
        }

        return resources;
    }

I was wondering if I should use string builder instead: 

static string GetResources(string header, string filter, string resourceTemplate)
    {
        string themeName;
        if (!TryGetHeaderValue(header, "theme", out themeName)) return "";

        var resourcesStringBuilder = new StringBuilder();
        var path = Path.Combine(HtmlPreview.BaseDirectory, themeName);

        foreach (var resource in Directory.GetFiles(path, filter))
        {
            resourcesStringBuilder.Append(String.Format(
                resourceTemplate,
                themeName,
                Path.GetFileName(resource)));
        }

        return resourcesStringBuilder.ToString();
    }

Resharper simply suggests it is possible to replace the original code with a LINQ statement:

    static string GetResources(string header, string filter, string resourceTemplate)
    {
        string themeName;
        if (!TryGetHeaderValue(header, "theme", out themeName)) return "";

        var path = Path.Combine(HtmlPreview.BaseDirectory, themeName);

        return Directory.GetFiles(path, filter)
            .Aggregate("", 
            (current, resource) => current + 
                String.Format(resourceTemplate, 
                themeName, 
                Path.GetFileName(resource)));
    }

Thank you! The original code with context is also on github.

share|improve this question

closed as off-topic by Mat's Mug♦, Gareth Rees, syb0rg, Jesse C. Slicer, Malachi Dec 20 '13 at 20:52

If this question can be reworded to fit the rules in the help center, please edit the question.
5  
This question appears to be off-topic because it is about understanding the fundamentals of the framework, not a code review. – Mat's Mug♦ Dec 20 '13 at 19:18
up vote 5 down vote

Strings in C# are immutable which means that this:

 resouces += string.Format(...);

will create 2 strings: one from string.Format and then the new string created by appending it to the existing string.

A StringBuilder will just keep a list of all the strings to append and create one result string at the end when you call ToString(). So instead of 2*n strings you only create n+1 strings. The Aggregate has the same problem.

There are some optimizations in the C# compiler which can transform string + string + string + ... sequences into Append calls on a StringBuilder but you should not really rely on this.

In the end it doesn't matter too much if your strings a short. However when you hit the 85kb limit then the strings will be placed on the large object heap which can cause problems because it doesn't get compacted and you can get holes. This can lead to weird out of memory situations (you can find more with google).

share|improve this answer
    
Thank you. These are local file paths so I doubt we'd hit the 85kb limit here but that is really good to know. – kush Dec 20 '13 at 18:16
up vote 3 down vote

Using string += in a loop is a very severe code smell, the worst possible solution both in terms of performance and memory fragmentation. Never do that, the StringBuilder class is designed exactly for such scenarios.

Edit The reason in short is that string concatenation with += will result in reallocation of the entire string for each loop cycle, whereas a StringBuilder is using a preallocated buffer which is filled with the individual parts.

share|improve this answer
2  
this sounds like the start to a very good answer, I am tempted to upvote it, as is, but I think you should elaborate a little bit more on why it is a code smell and how StringBuilder is designed to handle this situation. – Malachi Dec 20 '13 at 19:12
1  
@Malachi Here you go... – Thomas Weller Dec 21 '13 at 7:05

Not the answer you're looking for? Browse other questions tagged or ask your own question.