Assembling a user agent descriptor, implemented using StringBuilder vs String

Question

I think performance won't be a big issue here as the concatenation only happens once (2-3 at worse because of multiple threads). Out of the two methods, which one would you prefer, in terms of readability and performance? Is there any better way to accomplish the task of calculating desired string:uaInfo?

 public static string GetUserAgentInfo()
    {
        if (_uaInfo == null)
        {
            string carrier = DeviceNetworkInformation.CellularMobileOperator ?? "(No Network)";
            var sb = new StringBuilder();
            sb.Append("device-type=mobile");
            sb.Append(";os=Windows Phone");
            sb.Append(";os-version=" + Environment.OSVersion.Version);
            sb.Append(";app-name:" + GetAppName());
            sb.Append(";app-version=" + GetVersion());
            sb.Append(";carrier=" + HttpUtility.UrlEncode(carrier));
            sb.Append(";locale=" + Locale);
            _uaInfo = sb.ToString();
        }
        return _uaInfo;
    }

    public static String GetUserAgentInfoOld()
    {
        if (_uaInfo == null)
        {
            string carrier = DeviceNetworkInformation.CellularMobileOperator ?? "(No Network)";
            string temp = "";
            temp += String.Format("{0}={1};", "device-type", "mobile");
            temp += String.Format("{0}={1};", "os", "Windows Phone");
            temp += String.Format("{0}={1};", "os-version", Environment.OSVersion.Version);
            temp += String.Format("{0}={1};", "app-name", GetAppName());
            temp += String.Format("{0}={1};", "app-version", GetVersion());
            temp += String.Format("{0}={1};", "carrier", HttpUtility.UrlEncode(carrier));
            temp += String.Format("{0}={1}", "locale", Locale);
            _uaInfo = temp;
        }
        return _uaInfo;
    }

Answer 1

"Bug"

The two provided versions won't return the same String. In the first version you have ;app-name: and in the second you have "{0}={1};", "app-name". : versus =

Out of the above two methods, which one will you prefer?

As posted, neither.

At your first version you are doing concatenation of the strings too. A better way is to call the Append() method in a fluent way, because it is returning a StringBuilder object.

string carrier = DeviceNetworkInformation.CellularMobileOperator ?? "(No Network)";
var sb = new StringBuilder();
sb.Append("device-type=mobile")
  .Append(";os=Windows Phone")
  .Append(";os-version=").Append(Environment.OSVersion.Version)
  .Append(";app-name:").Append(GetAppName())
  .Append(";app-version=").Append(GetVersion())
  .Append(";carrier=").Append(HttpUtility.UrlEncode(carrier))
  .Append(";locale=").Append(Locale);
_uaInfo = sb.ToString();

In the second version you are doing too much formating of the String. So I would remove the first argument like

temp += "os=Windows Phone;";
temp += String.Format("os-version={0};", Environment.OSVersion.Version); 

I think both should be improved, because, although you use StringBuilder, your first version still creates too many String objects.

Calling

sb.Append(";os-version=" + Environment.OSVersion.Version);

will create 3 String objects befor the Append() method even is called.

Your second version will by calling

temp += String.Format("{0}={1};", "os-version", Environment.OSVersion.Version); 

create 4 String objects before concatenation to the temp string object which would create a new String.

I would use a StringBuilder because you are concatenation 12 strings which is similiar to using concatenation in a loop.

Answer 2

For a method like this, don't worry about performance until you have evidence that performance is a problem. Either way will almost certainly be fast enough.

I would suggest instead for readability that you separate the data (key-value pairs) from the construction of the string.

If the order of the pairs is not important, you can write

var data = new Dictionary<string, string>
{
    { "device-type", "mobile" },
    { "os", "Windows Phone" },
    ...
};
return string.Join(";", data.Select(kvp => kvp.Key + "=" + kvp.Value));

If order is important, use the slightly more verbose

var data = new[]
{
    new KeyValuePair<string, string>("device-type", "mobile"),
    new KeyValuePair<string, string>("os", "Windows Phone"),
    ...
};
return string.Join(";", data.Select(kvp => kvp.Key + "=" + kvp.Value));

Answer 3

Wait wait. I think you don't have an issue at all. StringBuilder has an AppendFormat method.

Not sure why the downvotes except for the terseness of my answer. OP seems torn between using SB for performance and string.Format() for readability, but he can have both with StringBuilder.AppendFormat()

Fixing up my answer: code sample.

You don't have to make the static info parameters. It makes it more readable.

builder.Append("device-type=mobile; os=Windows Phone;")
    .AppendFormat("os-version={0};", Environment.OSVersion.Version)
    .AppendFormat("app-name={0};", GetAppName())
    .AppendFormat("app-version={0};", GetVersion())
    .AppendFormat("carrier={0};", HttpUtility.UrlEncode(carrier));
    .AppendFormat("locale={0};", Locale);

You can also combine the AppendFormat statements as you like.

Answer 4

For the case you described, where performance is not a bottleneck, I believe it's all about personal taste.

In this SO discussion, it seems a majority prefers to use the 2nd approach.

I however, am a strong believer of staying consistent. If you use the StringBuilder class in other classes of your project, stick to using it.

However, When initializing a StringBuilder, you are going down in performance, so I wouldn't advise to use it for single concatenations. A guideline according to this article is to use a StringBuilder when four or more string concatenations take place (which is the case in your example.)

Answer 5

Generally, you should use StringBuilder when you have a variable number of concatenations or where the number of concatenations is potentially large.

For example, suppose you are reading a list of values from a database and concatenating them into a single string. You don't know how many values there are, so the size of the string has the potential to scale greatly.

The use of StringBuilder is justified here because the size of the internal character array will grow expontentially, massively reducing the number of times the entire array has to be reinitialised/copied if the number of concatenations grows large (versus having to initialise a new immutable string for every concatenation) - effectively O(logn) array initialisations vs. O(n).

In your case, you have a small, constant number of string concatenations, so the performance overhead for reinitialising those strings is likely to be negligible. You should favour simplicity and readability until you believe that performance is likely to be a concern, so simple string concatenation is probably your best option (though this is potentially subjective).