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.

I have a section of my code that seems to take a while during processing. Basically rawArray contains ~1.5 million rows and tempArray has around 1000 rows. I can see why it's slow as for each of the 1.5 million rows it has to loop through the tempArray and look for a match. Any hints for optimising this would be great!

foreach(string[] sArray in rawArray) {
    // Loop through tempArray and look for match in SRC and/or DST
    foreach (string[] sTemp in tempArray)
    {
        // SRC matches
        if (sArray[0] == sTemp[1])
        {                        
            // Set Octets value for upload
            if (sTemp[3] == "") 
            {
                sTemp[3] = "0";
            }

            // Convert to double and increment
            sTemp[3] = (System.Convert.ToDouble(sTemp[3]) + System.Convert.ToDouble(sArray[2])).ToString();
            // Date & Time
            sTemp[5] = date;
            sTemp[6] = time;
        }

        // DST matches
        if (sArray[1] == sTemp[1])
        {                        
            // Set Octets value for download
            if (sTemp[2] == "")
            {
                sTemp[2] = "0";
            }

            // Convert to double and increment
            sTemp[2] = (System.Convert.ToDouble(sTemp[2]) + System.Convert.ToDouble(sArray[2])).ToString();
            // Date & Time
            sTemp[5] = date;
            sTemp[6] = time;
        }                   
    }                
}
share|improve this question
2  
Could you please revise the title to state the purpose of this code? -1 until this is improved. –  Jamal Aug 28 at 6:06
    
I don't have a full answer, but your comments could use some work. They tell you nothing more than the code would... assuming the variable names were the same as those in the comments, which rarely seems to be the case. Also, sArray and sTemp look like some kind of class or struct that is being treated as an array. –  Magus Aug 28 at 14:48

4 Answers 4

up vote 3 down vote accepted

Well, you could build a dictionary for the temp array but it depends how often the key (index 1 of each temp row) is repeated. If you have very few repetitions then it could reduce the time because it only has to search through a subset of the 1000 temp rows.

Something along these lines:

Dictionary<string, List<string[]>> rowCache = new Dictionary<string, List<string[]>>();

foreach (var row in tempArray)
{
    List<string[]> keyRows;
    string key = row[1];
    if (!rowCache.TryGetValue(key, out keyRows))
    {
        keyRows = new List<string[]>();
        rowCache[key] = keyRows;
    }
    keyRows.Add(row);
}

Then your main loop becomes something like

// little helper method
public List<string[]> GetRows(string key)
{
    List<string[]> rows;
    return rowCache.TryGetValue(key, out rows) ? rows : new List<string[]>();
}

...

foreach (var row in rawArray)
{
    foreach (var srcRow in GetRows(row[0])
    {
       // perform your SRC logic here
    }

    foreach (var dstRow in GetRows(row[1])
    {
       // perform your DST logic here
    }
}

Another major speed improvement can probably be achieved by not using strings for entries which obviously aren't strings. You are doing a lot of double-to-string-and-back conversion which will be slow. If you can convert the input data into double when obtaining it, perform your calculations and then convert it back for output if necessary it should speed things up a lot as well.

share|improve this answer
    
Thanks a lot for the advice :) will post up some speed comparisons asap –  Alan Montefiore Aug 29 at 0:58

Some code issues I see:

  • Code in if (sArray[x] == sTemp[1]){} is obviously duplicated consider extracting a function
  • Conversion to ToDouble and ToString are CultureSpecific, so you code may work in USA and will be broken when invoked in Russia ( where . and , used )
  • Consider possible conversion exception

It is hard to say how to speed up code your code without looking at the whole code however consider the following

share|improve this answer

I'm not a fan of using empty quotes to designate an empty string.

// Set Octets value for upload
if (sTemp[3] == "") 

Using String.Empty makes it clear to Mr. Maintainer that this check is not a typo or bug. I would much prefer this.

if (sTemp[3] == String.Empty

Microsoft recommends against Hungarian notation. It was developed for weakly and untyped languages. C# is strongly typed and modern IDEs will tell you what type a variable is. That said, removing the prefix here would be a bad idea unless you give your variables more meaningful names. Variable names should tell Mr. Maintainer what data they contain, not what they are. Names like sArray are meaningless. It tells me nothing that the IDE won't. Never forget that even if Mr. Maintainer is you, you might will not remember what you were thinking 6 months from now. Write code as if the person who ends up maintaining it is a violent psychopath who knows where you live.

share|improve this answer

You can write common code outside of the if condition:

sTemp[5] = date;
sTemp[6] = time;

It will also improve your code a little bit.

share|improve this answer
    
Welcome to Code Review! We're looking for extended answers. Can you explain why this improves the code? –  mleyfman Aug 28 at 6:37
    
In example inside both condition same code is execute. Why to wrote same code multiple times. It is better to write once. –  Ankit Vaghela Aug 28 at 6:43

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.