current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite

The below logic is working fine. But is there any way to optimize this logic ?

I have a string1, string2,...,stringn and values of each strings are

string1 = "s1k1-s1k2-s1k3-....-s1kn | s1v1-s1v2-s1v3-....-s1vn";

string2 = "s2k1-s2k2-s2k3-....-s2kn | s2v1-s2v2-s2v3-....-s2vn";

                .............

stringn = "snk1-snk2-snk3-....-snkn | snv1-snv2-snv3-....-snvn";

Now, I want to populate the Dictionary<string, string>() with the key and values as follows.

var dict = new Dictionary<string, string>();
dict.Add(s1k1, s1v1);
dict.Add(s1k2, s1v2);
         :
         :
dict.Add(snkn, snvn);

Below is my sample code:

        string string1 = "s1k1-s1k2-s1k3-s1kn|s1v1-s1v2-s1v3-s1vn";
        string string2 = "s2k1-s2k2-s2k3-s2kn|s2v1-s2v2-s2v3-s2vn";
        string stringn = "snk1-snk2-snk3-snkn|snv1-snv2-snv3-snvn";

        var dict = new Dictionary<string, string>();

        new List<string> { string1, string2, stringn }.ForEach(str =>
        {
            var strSplit = str.Split('|');
            var strKeys = strSplit[0].Split('-');
            var strValues = strSplit[1].Split('-');
            strKeys.Zip(strValues, (key, value) => new { key, value }).ToList().ForEach(filed => dict.Add(filed.key, filed.value));
        });

Is it possible to optimize this code interms of Speed and Readability.

share|improve this question
2  
Optimize for what? Speed? Memory consumption? Readability? –  Jeff Mercado Oct 19 '12 at 3:55
    
-1: this code does not compile, and even if it did it wouldn't do what you say it does. –  ANeves Oct 19 '12 at 9:21
    
Hey Aneves, What is wrong in this post. You didn't understand the problem. –  Prasad Kanaparthi Oct 19 '12 at 17:57
    
Hello @PrasadKanaparthi, welcome to CodeReview. I understood the problem very well. But this is a place to ask for feedback on a specific _working_ piece of code from your project - see the FAQ. Code that has gross syntax and algorithm errors is not "working code". (I personally accept pseudo-code. Others don't.) After you fixed it your code is still missing at least a ); at the end and it still does not fill the dictionary, but that is good enough for me since you are new around here. I removed the -1. –  ANeves Oct 19 '12 at 18:49
    
The problem with the current code is that you're trying to (very wrongly) use Zip() to perform side effects (add items to an external dictionary). Not only that, it is never performed because the result of the zipping temp is never iterated over which would make it work. However, I would very strongly advise you or anyone from ever doing stuff like that. It will only lead to headaches. –  Jeff Mercado Oct 19 '12 at 18:57
show 1 more comment

1 Answer

up vote 6 down vote accepted

If you want to optimize for speed and possibly memory, just use regular loops to build up your dictionary, though it may hurt readability.

The idea is simple, scan the string for keys and values simultaneously. Don't extract substrings until you know what you need. Scan each token until you hit a delimiter and add the corresponding keys and values to your dictionary.

Assuming none of your keys or values do not contain any of your delimiters and everything is well formed and you have the same amount of values as you do keys for every string.

var myStrings = new[]
{
    "1-2-3-4-5|a-b-c-d-e",
    "10-20-30-40-50|aaa-bbb-ccc-ddd-eee",
};
var delims = new[] { '-', '|' };
var dict = new Dictionary<string, string>();
foreach (var str in myStrings)
{
    var sepIndex = str.IndexOf('|');

    var keyStart = 0;
    var valueStart = sepIndex + 1;
    while (keyStart < sepIndex)
    {
        // scan for the end of the token
        var keyEnd = str.IndexOfAny(delims, keyStart);
        var valueEnd = keyEnd < sepIndex
            ? str.IndexOfAny(delims, valueStart)
            : str.Length;

        // extract and add the tokens
        var key = str.Substring(keyStart, keyEnd - keyStart);
        var value = str.Substring(valueStart, valueEnd - valueStart);
        dict.Add(key, value);

        keyStart = keyEnd + 1;
        valueStart = valueEnd + 1;
    }
}

For readability, a declarative style works well and LINQ is king, just use it better.

var dict =
    (from str in myStrings
    let split = str.Split('|')
    let keys = split[0].Split('-')
    let values = split[1].Split('-')
    from pair in keys.Zip(values, (key, value) => new { key, value })
    select pair).ToDictionary(x => x.key, x => x.value);

As a side note, don't throw things into a list just for the sake of using the ForEach() method, you don't need the overhead of the list just to be able to interate over a collection through that method.

share|improve this answer
    
Thanks Jeff! for reply. Really good answer interms of Speed and Readability of the code. –  Prasad Kanaparthi Oct 19 '12 at 15:43
1  
+1 You did the work for Prasad since his code does not even work :) –  Gene S Oct 19 '12 at 16:41
    
Hey i just given the scenario with my logic. It works perfectly. But i just want to know is there better way to do that interms of speed and readability. –  Prasad Kanaparthi Oct 19 '12 at 17:58
2  
@PrasadKanaparthi When I run your code and put a break point after the ForEach statement, the "dict" variable is empty. –  Gene S Oct 19 '12 at 18:37
add comment

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.