views:

1004

answers:

9

I've started using StringBuilder in preference to straight concatenation, but it seems like it's missing a crucial method. So, I implemented it myself, as an extension:

public void Append(this StringBuilder stringBuilder, params string[] args)
{
    foreach (string arg in args)
        stringBuilder.Append(arg);
}

This turns the following mess:

StringBuilder sb = new StringBuilder();
...
sb.Append(SettingNode);
sb.Append(KeyAttribute);
sb.Append(setting.Name);

Into this:

sb.Append(SettingNode, KeyAttribute, setting.Name);

I could use sb.AppendFormat("{0}{1}{2}",..., but this seems even less preferred, and still harder to read. Is my extension a good method, or does it somehow undermine the benefits of StringBuilder? I'm not trying to prematurely optimize anything, as my method is more about readability than speed, but I'd also like to know I'm not shooting myself in the foot.

+3  A: 

Other than a bit of overhead, I don't personally see any issues with it. Definitely more readable. As long as you're passing a reasonable number of params in I don't see the problem.

KP
+57  A: 

I see no problem with your extension. If it works for you it's all good.

I myself prefere:

sb.Append(SettingNode)
  .Append(KeyAttribute)
  .Append(setting.Name);
Jesper Palm
+1 Do it this way! If they wrote it to return `this`, then this is completely what the designers intended.
Merlyn Morgan-Graham
fluid or fluent
kenny
@kenny are you asking what that style is called? AFAIK, it's known as "fluent interfaces."
Maxim Zaslavsky
I like the word fluid better!! please change ;)
kenny
http://www.martinfowler.com/bliki/FluentInterface.html
palswim
+1  A: 

I wouldn't say you're undermining it's efficiency, but you may be doing something inefficient when a more efficient method is available. AppendFormat is what I think you want here. If the {0}{1}{2} string being used constantly is too ugly, I tend to put my format strings in consts above, so the look would be more or less the same as your extension.

sb.AppendFormat(SETTING_FORMAT, var1, var2, var3);
Jimmy Hoffa
I don't know that AppendFormat is more efficient. Sure, it calls `Append` once, but under the hood it may be doing `StringBuildinger.Append(String.Format(...))`, which is instantiating an extra string. Of course it's conjecture either way until someone disassembles and sees what the IL actually looks like
STW
@STW: To be sure I can't say as though I know, I would just think one string.format though is more efficient than three sb.Appends.. Don't know..
Jimmy Hoffa
I'd be inclined to go with AppendFormat, particularly if the string is being displayed to the user; if the string needs to be reformatted (e.g., for localization), you don't need to change your code.
Eric Brown
Anything that uses format strings has to do some runtime parsing of the format string. There is no way this will be more efficient than multiple appends.
bruceboughton
A: 

Ultimately it comes down to which one results in less string creation. I have a feeling that the extension will result in a higher string count that using the string format. But the performance probably won't be that different.

Benjamin Anderson
+9  A: 

It's a little bit of overhead creating the extra array, but I doubt that it's a lot. You should measure

If it turns out that the overhead of creating string arrays is significant, you can mitigate it by having several overloads - one for two parameters, one for three, one for four etc... so that only when you get to a higher number of parameters (e.g. six or seven) will it need to create the array. The overloads would be like this:

public void Append(this builder, string item1, string item2)
{
    builder.Append(item1);
    builder.Append(item2);
}

public void Append(this builder, string item1, string item2, string item3)
{
    builder.Append(item1);
    builder.Append(item2);
    builder.Append(item3);
}

public void Append(this builder, string item1, string item2,
                   string item3, string item4)
{
    builder.Append(item1);
    builder.Append(item2);
    builder.Append(item3);
    builder.Append(item4);
}

// etc

And then one final overload using params, e.g.

public void Append(this builder, string item1, string item2,
                   string item3, string item4, params string[] otherItems)
{
    builder.Append(item1);
    builder.Append(item2);
    builder.Append(item3);
    builder.Append(item4);
    foreach (string item in otherItems)
    {
        builder.Append(item);
    }
}

I'd certainly expect these (or just your original extension method) to be faster than using AppendFormat - which needs to parse the format string, after all.

Note that I didn't make these overloads call each other pseudo-recursively - I suspect they'd be inlined, but if they weren't the overhead of setting up a new stack frame etc could end up being significant. (We're assuming the overhead of the array is significant, if we've got this far.)

Jon Skeet
Visual Studio seems to want to use `params string[] args` instead of `string arg0, string arg1`, etc. Do I have to do `string arg0, string arg1, ..., params string[] moreArgs` to get this to work?
Daniel Rasmussen
@Jon No offense, but the multiple overloads looks heinous. At least make it a regression call for each extra overload, and why not just use params string[] args?
Lucas B
@Lucas: The point of using some overloads with multiple parameters is to avoid the overhead of the array which is created every time you use a method with a `params` parameter. Yes, it may well be overkill - but this is a performance question, hence the answer. (I stated at the start that it won't be much overhead.) As for making one call another - I guess that wouldn't hurt *if* it's being inlined... but otherwise it's an extra stack frame each time.
Jon Skeet
@Daniel: Your comment isn't clear, but yes - you'd end up with one method call with various `string` parameters to start with, and one `params string[] args` at the end, which would only be used if you had a lot of parameters. But as I said in the start of my answer, I suspect there isn't much overhead anyway. I'll edit my answer to make that clearer.
Jon Skeet
*@Jon* - Sorry for the unclear comment; it's hard to fit nice code in a comment like that. From the looks of your edit, you understood me perfectly, though.
Daniel Rasmussen
+27  A: 

Questions like this can always be answered with a simple test case.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Diagnostics;

namespace SBTest
{
    class Program
    {
        private const int ITERATIONS = 1000000;

        private static void Main(string[] args)
        {
            Test1();
            Test2();
            Test3();
        }

        private static void Test1()
        {
            var sw = Stopwatch.StartNew();
            var sb = new StringBuilder();

            for (var i = 0; i < ITERATIONS; i++)
            {
                sb.Append("TEST" + i.ToString("00000"),
                          "TEST" + (i + 1).ToString("00000"),
                          "TEST" + (i + 2).ToString("00000"));
            }

            sw.Stop();
            Console.WriteLine("Testing Append() extension method...");
            Console.WriteLine("--------------------------------------------");
            Console.WriteLine("Test 1 iterations: {0:n0}", ITERATIONS);
            Console.WriteLine("Test 1 milliseconds: {0:n0}", sw.ElapsedMilliseconds);
            Console.WriteLine("Test 1 output length: {0:n0}", sb.Length);
            Console.WriteLine("");
        }

        private static void Test2()
        {
            var sw = Stopwatch.StartNew();
            var sb = new StringBuilder();

            for (var i = 0; i < ITERATIONS; i++)
            {
                sb.Append("TEST" + i.ToString("00000"));
                sb.Append("TEST" + (i+1).ToString("00000"));
                sb.Append("TEST" + (i+2).ToString("00000"));
            }

            sw.Stop();    
            Console.WriteLine("Testing multiple calls to Append() built-in method...");
            Console.WriteLine("--------------------------------------------");
            Console.WriteLine("Test 2 iterations: {0:n0}", ITERATIONS);
            Console.WriteLine("Test 2 milliseconds: {0:n0}", sw.ElapsedMilliseconds);
            Console.WriteLine("Test 2 output length: {0:n0}", sb.Length);
            Console.WriteLine("");
        }

        private static void Test3()
        {
            var sw = Stopwatch.StartNew();
            var sb = new StringBuilder();

            for (var i = 0; i < ITERATIONS; i++)
            {
                sb.AppendFormat("{0}{1}{2}",
                    "TEST" + i.ToString("00000"),
                    "TEST" + (i + 1).ToString("00000"),
                    "TEST" + (i + 2).ToString("00000"));
            }

            sw.Stop();
            Console.WriteLine("Testing AppendFormat() built-in method...");
            Console.WriteLine("--------------------------------------------");            
            Console.WriteLine("Test 3 iterations: {0:n0}", ITERATIONS);
            Console.WriteLine("Test 3 milliseconds: {0:n0}", sw.ElapsedMilliseconds);
            Console.WriteLine("Test 3 output length: {0:n0}", sb.Length);
            Console.WriteLine("");
        }
    }

    public static class SBExtentions
    {
        public static void Append(this StringBuilder sb, params string[] args)
        {
            foreach (var arg in args)
                sb.Append(arg);
        }
    }
}

On my PC, the output is:

Testing Append() extension method...
--------------------------------------------
Test 1 iterations: 1,000,000
Test 1 milliseconds: 1,080
Test 1 output length: 29,700,006

Testing multiple calls to Append() built-in method...
--------------------------------------------
Test 2 iterations: 1,000,000
Test 2 milliseconds: 1,001
Test 2 output length: 29,700,006

Testing AppendFormat() built-in method...
--------------------------------------------
Test 3 iterations: 1,000,000
Test 3 milliseconds: 1,124
Test 3 output length: 29,700,006

So your extension method is only slightly slower than the Append() method and is slightly faster than the AppendFormat() method, but in all 3 cases, the difference is entirely too trivial to worry about. Thus, if your extension method enhances the readability of your code, use it!

Chris
I feel like the three `"TEST" + i.ToString("00000")` per append would dwarf the time it takes to append it to a `StringBuilder`, and should probably be run with constant strings instead, just to get a better picture.
Daniel Rasmussen
Great test btw!
KP
Evidence - always good.
ChrisF
+1 My previous comment was just being nitpicky - but yes, it's a good test!
Daniel Rasmussen
@Daniel, I was just creating dynamic strings to avoid any optimizations string interning might give. Not sure if it really affects the test, but I suppose one could run the test with constants to double check. :P
Chris
*@Chris* - Good point about the interning; I hadn't thought of that.
Daniel Rasmussen
+1  A: 

From a clarity perspective, your extension is ok.

It would probably be best to simply use the .append(x).append(y).append(z) format if you never have more than about 5 or 6 items.

StringBuilder itself would only net you a performance gain if you were processing many thousands of items. In addition you'll be creating the array every time you call the method.

So if you're doing it for clarity, that's ok. If you're doing it for efficiency, then you're probably on the wrong track.

user98071
+1  A: 

I haven't tested recently, but in the past, StringBuilder was actually slower than plain-vanilla string concatenation ("this " + "that") until you get to about 7 concatenations.

If this is string concatenation that is not happening in a loop, you may want to consider if you should be using the StringBuilder at all. (In a loop, I start to worry about allocations with plain-vanilla string concatenation, since strings are immutable.)

Jason
+1 for concatenation, good point!
Lucas B
A: 

Chris,

Inspired by this Jon Skeet response (second answer), I slightly rewrote your code. Basically, I added the TestRunner method which runs the passed-in function and reports the elapsed time, eliminating a little redundant code. Not to be smug, but rather as a programming exercise for myself. I hope it's helpful.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Diagnostics;

namespace SBTest
{
  class Program
  {
    private static void Main(string[] args)
    {
      // JIT everything
      AppendTest(1);
      AppendFormatTest(1);

      int iterations = 1000000;

      // Run Tests
      TestRunner(AppendTest, iterations);
      TestRunner(AppendFormatTest, iterations);

      Console.ReadLine();
    }

    private static void TestRunner(Func<int, long> action, int iterations)
    {
      GC.Collect();

      var sw = Stopwatch.StartNew();
      long length = action(iterations);
      sw.Stop();

      Console.WriteLine("--------------------- {0} -----------------------", action.Method.Name);
      Console.WriteLine("iterations: {0:n0}", iterations);
      Console.WriteLine("milliseconds: {0:n0}", sw.ElapsedMilliseconds);
      Console.WriteLine("output length: {0:n0}", length);
      Console.WriteLine("");
    }

    private static long AppendTest(int iterations)
    {
      var sb = new StringBuilder();

      for (var i = 0; i < iterations; i++)
      {
        sb.Append("TEST" + i.ToString("00000"),
                  "TEST" + (i + 1).ToString("00000"),
                  "TEST" + (i + 2).ToString("00000"));
      }

      return sb.Length;
    }

    private static long AppendFormatTest(int iterations)
    {
      var sb = new StringBuilder();

      for (var i = 0; i < iterations; i++)
      {
        sb.AppendFormat("{0}{1}{2}",
            "TEST" + i.ToString("00000"),
            "TEST" + (i + 1).ToString("00000"),
            "TEST" + (i + 2).ToString("00000"));
      }

      return sb.Length;
    }
  }

  public static class SBExtentions
  {
    public static void Append(this StringBuilder sb, params string[] args)
    {
      foreach (var arg in args)
        sb.Append(arg);
    }
  }
}

Here's the output:

--------------------- AppendTest -----------------------
iterations: 1,000,000
milliseconds: 1,274
output length: 29,700,006

--------------------- AppendFormatTest -----------------------
iterations: 1,000,000
milliseconds: 1,381
output length: 29,700,006
Matthew Sposato