# Need help to optimise my C# code

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])
{
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;
}
}
}

-
Could you please revise the title to state the purpose of this code? -1 until this is improved. – Jamal Aug 28 '14 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 '14 at 14:48

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;
}
}


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.

-
Thanks a lot for the advice :) will post up some speed comparisons asap – Alan Montefiore Aug 29 '14 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

-

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.

-

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.

-
Welcome to Code Review! We're looking for extended answers. Can you explain why this improves the code? – mleyfman Aug 28 '14 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 '14 at 6:43