11
\$\begingroup\$

I'm trying to write a program that has a for loop, inside of which I have lambda expression to get a certain value. The loop has about 20,000 iterations. Including this lambda expression increases the execution time significantly. If I include it, it takes about 20 mins for the loop to finish; without it, it takes just 10 seconds.

Here's what the lambda expression looks like:

code = countries.Find(c => c.ISO2Code == iso2).Country_Code

Same expression using Linq:

code = (from a in countries
        where a.ISO2Code == iso2
         select a.Country_Code).First();

Here, the list countries is cached for the loop starts, so it's not like the query is made to the DB. What's the reason for this time costly expression?

Here's the full code for the loop:

for (int i = 1; i < count - 1; i++)
{
    if (!timer.IsRunning)
        timer.Start();

    var d = line[i];
    var row = d.Replace("\n", "");
    var values = row.Split(',');

    if (values.Length < 3)
    {
        return;
    }


    if (values.Length >= 2)
    {
        if (values[2] != null && values[2] != "")
        {
            string contactno = Utils.RemoveSpecialCharacters(values[2]);


            if (model.NumberFormat == 1)
            {
                Country cntry =
                    countries.Where(x => x.Country_Code == model.CountryCode)
                        .FirstOrDefault();

            contactno = utils.GetValidNumberWithCountryCode(contactno, cntry.ISO2Code);
            }

            int? code = null;
            try
            {
                contactno = "+" + contactno;
                try
                {
                //this is commented
                ph = phoneUtil.parseAndKeepRawInput(contactno, "US");
                if (phoneUtil.isValidNumber(ph))
                {
                    string iso2 = phoneUtil.getRegionCodeForNumber(ph);

                    if (!string.IsNullOrEmpty(iso2))
                    {
                        code = countries.Find(c => c.ISO2Code == iso2).Country_Code;
                        //code = (from a in countries
                        //        where a.ISO2Code == iso2
                        //        select a.Country_Code).First();
                    }
                }
            }
            catch
                {
                    code = null;
                }
            }
            catch
            {
            }

            if (code != null && utils.IsValidNumber(contactno))
            {
                contactno = utils.GetValidNumber(contactno);

                bool match = false;
                foreach (var item in existingList)
                {
                    if (item.ContactNumber.ToString() == contactno)
                    {
                        ExistingnumberCount += 1;
                        match = true;
                        break;
                    }
                }
                if (!match)
                {
                    validNumberCount += 1;
                    Contact_List cl = new Contact_List();
                    cl.Customer_Code = model.Customer_Code;
                    try
                    {
                        string fname = values[0];
                        if (fname.Length > 50)
                        {
                            fname = fname.Substring(0, 50);
                        }
                        cl.First_Name = fname;
                    }
                    catch
                    {
                    }
                    try
                    {
                        string lname = values[1];
                        if (lname.Length > 50)
                        {
                            lname = lname.Substring(0, 50);
                        }
                        cl.Last_Name = lname;
                    }
                    catch
                    {
                    }

                    try
                    {
                        string CF1 = values[3];
                        if (CF1.Length > 50)
                        {
                            CF1 = CF1.Substring(0, 50);
                        }
                        cl.CF1 = CF1;
                    }
                    catch
                    {
                    }
                    try
                    {
                        string CF2 = values[4];
                        if (CF2.Length > 50)
                        {
                            CF2 = CF2.Substring(0, 50);
                        }
                        cl.CF2 = CF2;
                    }
                    catch
                    {
                    }
                    try
                    {
                        string CF3 = values[5];
                        if (CF3.Length > 50)
                        {
                            CF3 = CF3.Substring(0, 50);
                        }
                        cl.CF3 = CF3;
                    }
                    catch
                    {
                    }
                    try
                    {
                        string CF4 = values[6];
                        if (CF4.Length > 50)
                        {
                            CF4 = CF4.Substring(0, 50);
                        }
                        cl.CF4 = CF4;
                    }
                    catch
                    {
                    }

                    cl.CountryCode = code;
                    cl.Contact_No = contactno.Replace("+", "");
                    cl.Is_Active = true;
                    cl.Create_Date = DateTime.Now;
                    cl.Opt_Status_Code = 0;
                    try
                    {
                        string Contact_Email = values[7];
                        if (Contact_Email.Length > 50)
                        {
                            Contact_Email = Contact_Email.Substring(0, 50);
                        }
                        cl.Contact_Email = Contact_Email;
                    }
                    catch
                    {
                    }
                } 
            }
            else
            {
                InvalidnumberCount += 1;
            }
        }
    }

    Debug.WriteLine("Total Execution Time: " + (double)(timer.ElapsedMilliseconds / 1000));
    Debug.WriteLine("No of iterations completed: " + i);
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Now that you've added a whole working program you should change your title to be more descriptive of the purpose of the code. \$\endgroup\$ – Denis Dec 20 '16 at 10:33
  • 3
    \$\begingroup\$ You have so many insane try/catch blocks there. Why don't you check the indexes before accessing the array or getting the substrings? \$\endgroup\$ – t3chb0t Dec 20 '16 at 10:36
  • 2
    \$\begingroup\$ @RubberDuck Where().First() does not need to find all matches. Where() constructs a lazy enumerable (and doesn't actually find anything) and First() merely enumerates the first item out of it. As soon as the first item is found, it will stop. There will be some additional overhead compared to Find() what with the extra objects and method calls involved, but it definitely won't look at more items than it has to. \$\endgroup\$ – Kyle Dec 20 '16 at 15:33
  • 2
    \$\begingroup\$ @RubberDuck Indeed, there's no reason it should be. They will both do the same exact thing: iterate over the collection, apply the supplied delegate, return the current value when that delegate returns true. \$\endgroup\$ – Kyle Dec 20 '16 at 16:14
  • 1
    \$\begingroup\$ You should use this: codereview.stackexchange.com/contact to merge your user accounts. \$\endgroup\$ – Heslacher Dec 21 '16 at 5:53
41
\$\begingroup\$

Your loop is close to 200 lines, that's waaaay too long. That's long for a full class, let alone one method, let alone part of one method.


Why do you use for (int i = 1; i < count - 1; i++) when you don't do anything with i? Why not use foreach(var line in lines)? Oh wait, I see, you've called your collection line -- a bad name, since this doesn't tell me it is a collection of lines.


You assign d, a name that conveys absolutely nothing, only to use that to execute var row = d.Replace("\n", "");. Why not do that in one line?


Everything inside if (values.Length >= 2) should be moved to a separate method; matter of fact make that everything inside if (values[2] != null && values[2] != ""). Also, don't you know about string.IsNullOrEmpty()?


values[0], values[1], values[2], etc. are meaningless. It would be far better to convert each line into a class with properly named properties.


Then suddenly we encounter if (model.NumberFormat == 1), yet model is nowhere defined. I assume this comes from outside the for loop? If so, then why is this code located here?


Property names should not contain an underscore: Country_Code.


Logic like "retrieve contactno", "retrieve code", etc. should all be a method of their own.


You are using try...catch waaaay too much. I mean, this is just nuts:

int? code = null;
try
{
    contactno = "+" + contactno;
    try
    {
        //this is commented
        ph = phoneUtil.parseAndKeepRawInput(contactno, "US");
        if (phoneUtil.isValidNumber(ph))
        {
            string iso2 = phoneUtil.getRegionCodeForNumber(ph);

            if (!string.IsNullOrEmpty(iso2))
            {
                code = countries.Find(c => c.ISO2Code == iso2).Country_Code;
                //code = (from a in countries
                //        where a.ISO2Code == iso2
                //        select a.Country_Code).First();
            }
        }
    }
    catch
    {
        code = null;
    }
}
catch
{
}

Do you seriously expect contactno = "+" + contactno; to throw an exception?


Why is there a comment saying //this is commented?


This:

bool match = false;
foreach (var item in existingList)
{
    if (item.ContactNumber.ToString() == contactno)
    {
        ExistingnumberCount += 1;
        match = true;
        break;
    }
}

can basically be replaced by match = existingList.Any(x => x.ContactNumber.ToString() == contactno);.


At that point I'm not even halfway through and I have to give up. You need to seriously refactor this code. Each property you need to retrieve should have its own method, and in 99% of those cases there shouldn't be an empty try...catch block.

WRT your question: consider converting countries to a Dictionary<string, string>, e.g. var countryCodeByIso2 = countries.ToDictionary(x => x.ISO2Code, x => x.Country_Code); and then use TryGetValue(). But please, first refactor your code.

\$\endgroup\$
  • \$\begingroup\$ I joined this community just so that I could up-vote this answer. \$\endgroup\$ – Bernhard Hofmann Dec 21 '16 at 11:28
  • \$\begingroup\$ Maybe add string.IsNullOrWhiteSpace() to your vocabulary too, this doesn't just check for empty strings, but all whitespace strings as well. \$\endgroup\$ – AsheraH Dec 21 '16 at 13:02
17
\$\begingroup\$

Instead of using linq lookups for the lists you should map your data into dictionaries, which are MUCH faster to access specific instance of your data, memorywise you might take a hit but the mapping is just onetime.

var countries = new List<Country>();
var countriesByCode = countries.ToDictionary(c => c.Country_Code, c => c);

Country cntry;
if (countriesByCode.TryGetValue(countryCody, out cntry))
{
    contactno = utils.GetValidNumberWithCountryCode(contactno, cntry.ISO2Code);
}

Consider this sample on what is the performance difference.

var stringList = Enumerable.Range(0, 50000).Select(s => new Tuple<int, string>(s, s.ToString())).ToList();
var dictionary= stringList.ToDictionary(s => s.Item1, s => s);

var sw = new Stopwatch();
sw.Start();
for (var i = 50000; i > 0; i--)
{
    var t = stringList.FirstOrDefault(s => s.Item1 == i);
    if (t != null)
    {
    }
}
Console.WriteLine(sw.Elapsed);

sw.Restart();
for (var i = 50000; i > 0; i--)
{
    Tuple<int, string> t;
    if (dictionary.TryGetValue(i, out t))
    {
    }
}
Console.WriteLine(sw.Elapsed);

It takes about 20seconds to perform the lookups through the list, but under a millisecond to perform the same with dictionary lookups. Memory is consumed a bit more however.

\$\endgroup\$
  • \$\begingroup\$ I doubt there are 50000 countries but I tested your code with just 500 and that is consistent with performance reported by OP. \$\endgroup\$ – paparazzo Dec 20 '16 at 18:33
  • \$\begingroup\$ List lookup is O(N), dictionary / hash lookup is O(1). Unless N is guaranteed very small every time, using a dictionary is always going to win, and the memory overhead is rarely if ever a problem these days. \$\endgroup\$ – nigel222 Dec 21 '16 at 11:28
10
\$\begingroup\$
if (values.Length < 3)
{
    return;
}


if (values.Length >= 2)
{  

How should in the second if the values.Length ever be anything than > 2 ? That second if is superflous and doesn't add anything but a level of intendation to the code and makes it less readable.

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

Be sure that countries is cached
You could close the DB connection before the loop to test

Move this to before the loop

if (!timer.IsRunning)
    timer.Start();

-

values[2] != null && values[2] != ""

could be

!String.IsNullOrEmpty(values[2])

Duplicate catch

Some indents wrong

How would this ever fail?
You know Length >= 2

try 
{   string fname = values[0];
    if (fname.Length > 50)
    {
       fname = fname.Substring(0, 50);
    }
    cl.First_Name = fname;
 }

Go with

 cl.First_Name = values[0].Length > 50 ? values[0].Substring(0, 50) : values[0];

 if(values.Length >= 3)
 {
     cl.CF2 = values[4].Length > 50 ? values[4].Substring(0, 50) : values[4];
 }

You create a new Contact_List and do not actually use it for anything

Contact_List cl = new Contact_List();

convert to double after you divide adds not value

(double)(timer.ElapsedMilliseconds / 1000)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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