8
\$\begingroup\$

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;
    }
\$\endgroup\$

5 Answers 5

10
\$\begingroup\$

"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.

\$\endgroup\$
2
  • \$\begingroup\$ "Why I consider both should be improved is, because your first version will nevertheless you are using the StringBuilder object, create too many String objects." This sentence makes no grammatical sense, what are you trying to say? \$\endgroup\$
    – Pharap
    Jan 19, 2015 at 4:40
  • \$\begingroup\$ @Pharap rephrased the sentence. \$\endgroup\$
    – Heslacher
    Jan 19, 2015 at 6:07
14
\$\begingroup\$

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));
\$\endgroup\$
3
  • 3
    \$\begingroup\$ +1. This, because it doesn't repeat the x=y; pattern (which can lead to typos such as the app-name: bug). Note: Tuple.Create would be less verbose than new KeyValuePair<string, string> albeit being less descriptive. \$\endgroup\$
    – Heinzi
    Jan 19, 2015 at 6:30
  • \$\begingroup\$ You could also use nested arrays to save some keystrokes, like var data = new[] { new []{"device-type", "mobile"}, ...} and create the string with string.Join(";", data.Select(kvp => string.Join("=", kvp)));. Using KeyValuePair is quite verbose (but also less prone to errors). \$\endgroup\$
    – sloth
    Jan 19, 2015 at 11:36
  • 1
    \$\begingroup\$ For this kind of lists, I like to have TupleList<T1, T2>: it inherits from List<Tuple<T1, T2>> and has an Add(T1, T2) method, so you can write e.g. new TupleList<string, string> { { "device-type", "mobile" }, … }, just like you do with a Dictionary. \$\endgroup\$
    – svick
    Jan 19, 2015 at 21:56
10
\$\begingroup\$

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.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ Very readable. Maintains best practice of using StringBuilder for concatenation. Excellent use of available syntax. \$\endgroup\$
    – mdisibio
    Jan 19, 2015 at 6:43
4
\$\begingroup\$

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

\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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