Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Could this code:

var a = new StringBuilder();
var b = new StringBuilder();
var kvps = new List<KeyValuePair<Guid, object>>
{
    new KeyValuePair<Guid, object>(Guid.NewGuid(), 1),
    new KeyValuePair<Guid, object>(Guid.NewGuid(), "hello")
};
for (var c = 0; c < kvps.Count; c++)
{
    a.Append(kvps[c].Key);
    b.Append(kvps[c].Value);
    if (c >= kvps.Count - 1) continue;
    a.Append(",");
    b.Append(",");
}

be improved in terms of performance to achieve this:

a = e1b7978c-127f-4a5f-a17b-737e03484172,cbbd1573-950c-4ca4-94b7-72a08b4a61d0
b = 1,hello
share|improve this question
3  
To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question –  Simon André Forsberg Jun 25 at 14:36
    
Is the code not explicit enough? –  csetzkorn Jun 25 at 14:38
1  
It might be actually, especially considering you have two answers already. Consider it next time though. And welcome to Code Review! :) –  Simon André Forsberg Jun 25 at 14:40

4 Answers 4

up vote 10 down vote accepted

Performance here will only matter if the input List is large. There are two things that can be done to make the output loop faster:

  1. initialize the starting capacity of the StringBuilder to match your expectations of what size it will need to be.

    In this case, you know the GUUID's are 36 characters long, with a comma, makes it 37. You should initialize that StringBuilder to ... new StringBuilder(37 * kvps.Count);. Similarly, if you have an approximate size of the average length for the other Stringbuilder, then double-it, and use it as the initial capacity constructor.

  2. If the loop happens (very) often, you can get a very slight performance improvement by doing the following (removing the if condition from inside the loop):

    foreach (KeyValuePair kv in kvps)
    {
        a.Append(kv.Key);
        a.Append(",");
        b.Append(kv.Value);
        b.Append(",");
    }
    // remove the trailing comma.
    if (kvps.Count > 0) {
        a.Length = a.Length - 1;
        b.Length = b.Length - 1;
    }
    

It should go without saying that the names a and b are bad names for the StringBuilder instances ....

share|improve this answer
    
Thanks. Nice trick to remove the trailing comma, which allows me to use foreach. Please note the names were just chosen for this example but yes even here I should use better names in the future (-: –  csetzkorn Jun 25 at 14:22
1  
I suspect that the JIT compiler (which makes intelligent guesses about ifs) will probably perform almost exactly as well as this method, but you have to test it to make sure. –  Snowbody Jun 25 at 14:47

If you insist that the type remain List<KeyValuePair<Guid, object>> you could still make use of String.Join() by using Select(), a LINQ extension method::

// at top of file: using System.Linq;

String keys = String.Join(",", kvps.Select(kv=>kv.Key.ToString()));
String values = String.Join(",", kvps.Select(kv=>kv.Value.ToString()));

The downside is that there are potentially two passes over the list (I say "potentially" because there's a good chance that the optimizer in the compiler or JIT may detect the parallelism and exploit it).

share|improve this answer

Since you are essentially performing a fold of the list and returning a tuple you could express just that in the code which would also be rather short and still only loop trough the list once

var builders = kvps.Aggregate(Tuple.Create(new StringBuilder(),new StringBuilder(),
                        (acc,kv) => {
                                      acc.Item1.Append("," + kv.Key);
                                      acc.Item2.Append("," + kv.Value)
                                      return acc;
                                    });
a = builders.Item1.ToString().Substring(1);
b = builders.Item2.ToString().Substring(1);

Whether or not this can compare speedwise with other implementations depend on how well the optimizer can optimize the code

share|improve this answer
    
I really like this method, but unfortunately the code required to implement it is a little verbose. Also I think that for memory alignment purposes it's more efficient to have the "," be a suffix instead of a prefix. –  Snowbody Jun 26 at 16:15
    
Yeah unfortunately it is verbose. It's a shame the same implementation in F# is a lot nicer (The inspiration came from there) tuples just aren't natural in C# –  Rune FS Jun 26 at 17:17

As a GUID should be unique you should think about using a Dictionary<GUID,object> instead of your List<KeyValuePair<GUID,object>> .

If using a Dictionary is possible you can get the comma separated list by using the String.Join() method.

Dictionary<Guid, object> kvps = new Dictionary<Guid, object>();

kvps.Add(Guid.NewGuid(), 1);
kvps.Add(Guid.NewGuid(), "hello");

String keys = String.Join(",", kvps.Keys);
String values = String.Join(",", kvps.Values);
share|improve this answer
1  
This won't work if order is important, as the order of the keys in the dictionary is unspecified. –  mjolka Jun 25 at 14:30
    
Also it would loop twice potentially ... –  csetzkorn Jun 25 at 14:32
    
It may loop twice or it may not; it depends how intelligent the optimizer (either at compilation or JIT) is feeling that day. –  Snowbody Jun 25 at 15:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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