CODEWITHSUNDEEP
c#optimization
Sha Le
Sha Le

Reputation: 231

How can this code be optimized further?

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?

Upvotes: 1

Views: 406

Answers (9)

Ian Mercer
Ian Mercer

Reputation: 39277

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);

   ...

Upvotes: 0

Zach Johnson
Zach Johnson

Reputation: 24232

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.

Upvotes: 1

Igby Largeman
Igby Largeman

Reputation: 16747

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);
}

Upvotes: 8

McAden
McAden

Reputation: 13972

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.

Upvotes: 0

Rosdi Kasim
Rosdi Kasim

Reputation: 25966

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);
}

Upvotes: 0

tsilb
tsilb

Reputation: 8037

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.

Upvotes: 2

Kobi
Kobi

Reputation: 138017

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

Upvotes: 2

Ben S
Ben S

Reputation: 69342

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.

Upvotes: 12

Carl Manaster
Carl Manaster

Reputation: 40336

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

Upvotes: 5

Related Questions