c#, can you think of anymore optimization in this code?

Question

Hi All: Look at the following code:

StringBuilder row = TemplateManager.GetRow("xyz"); // no control over this method 
StringBuilder rows = new StringBuilder();

foreach(Record r in records)
{
    StringBuilder thisRow = new StringBuilder(row.ToString());
    thisRow.Replace("%firstName%", r.FirstName)
       .Replace("%lastName%", r.LastName)
          // all other replacement goes here
       .Replace("%LastModifiedDate%", r.LastModifiedDate);

    //finally append row to rows
    rows.Append(thisRow);
}

Currently 3 StringBuilders and row.ToString() is inside a loop. Is there any room for further optimization here?

Thanks a bunch. :-)

Answer 1

You could take row.ToString() out of the loop; row never changes inside the loop.

Answer 2

Is this a bottleneck in your application? Have you profiled it and know it needs optimization?

Don't optimize unless you need to. Pre-mature optimization leads to less maintainable code that may or may not even be optimized.

Answer 3

I'd argue that row should be a string, not a StringBuilder.
More of a design comment than an optimization.

Answer 4

Not a big deal if the row is small, but you could try combining those three replaces into a single replace action.

Edit: You could do this by walking through each character in the StringBuilder using a loop. Since it appears that you are replacing based on %name%, you could walk until you see a % and capture the text until you come across another %. If the captured text matches one of your replacement keys, substitute its replacement value and ignore the text until you come across the next % and repeat the capture process. Otherwise, consider try the capture process from that last % you encountered. That is roughly what you would want to do. Of course, any text that doesn't match a key would have to be appended to the output StringBuilder.

Answer 5

StringBuilders generally only provide great improvements when concatenating strings into very large strings, not very large quantities of small ones. If your rows are small you might consider clocking this code against normal strings.

i.e.

string row = TemplateManager.GetRow("xyz").ToString();
foreach (Record r in records) 
    rows.Append(row
        .Replace("%firstName%", r.FirstName)
        .Replace("%lastName", r.LastName)
        // ....
        );

Or just use XSLT.

Answer 6

I thought it might be quicker to replace thisRow with a string, but I did some tests and StringBuilder won.

String.Replace() creates a new string, which is expensive. StringBuilder.Replace() modifies the string in its buffer, which is less expensive. In my test I used a string of 500 chars and performed 6 Replace()'s. StringBuilder was about 5% faster, including creating a new one each iteration.

However, one thing you should do is move the row.ToString() outside the loop. Calling it for each record is wasting cycles by creating a new string.

String row = TemplateManager.GetRow("xyz").ToString();
StringBuilder rows = new StringBuilder(); 

foreach(Record r in records) 
{ 
    StringBuilder thisRow = new StringBuilder(row);  
    thisRow.Replace("%firstName%", r.FirstName) 
                   .Replace("%lastName%", r.LastName) 
                   .Replace("%LastModifiedDate%", r.LastModifiedDate); 
    rows.Append(thisRow);
}    

Answer 7

StringBuilder row = TemplateManager.GetRow("xyz"); // no control over this method 
StringBuilder rows = new StringBuilder();

foreach(Record r in records)
{
    row.Replace("%firstName%", r.FirstName)
       .Replace("%lastName%", r.LastName)
          // all other replacement goes here
       .Replace("%LastModifiedDate%", r.LastModifiedDate);

    //finally append row to rows
    rows.Append(row);
}

Answer 8

Depending on what "Records" is and how many there are, you might look at using for, instead of foreach.

I've found Jon Skeet's benchmarking analysis of for vs foreach interesting.

Answer 9

Like Ben said, don't do it!

But if you must, the obvious optimization is to move the calculation of all the indexes for where to copy and where to substitute out of the loop entirely. 'rows' is constant so the positions of each substitutable parameter within it are constant.

If you do that the loop becomes just a bunch of SubString() and concatenation operations and it runs ~3x faster for the case with just one of each in the template in that order.

Or, for an even easier (but still questionable) optimization that can easily handle repeats (but not {#} values in the template string!) ...

string format = row.Replace("%firstname%,"{0}").Replace("%...

for ...

   string thisRow = string.format(format, r.FirstName, r.LastName, r.LastModifiedDate);

   ...