HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMajor

Country code lookup for each line in a CSV file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fileeachlinecountrycsvforcodelookup

Problem

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 = 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();

Solution

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 = 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, e.g. var countryCodeByIso2 = countries.ToDictionary(x => x.ISO2Code, x => x.Country_Code); and then use TryGetValue(). But please, first refactor your code.

Code Snippets

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
{
}
bool match = false;
foreach (var item in existingList)
{
    if (item.ContactNumber.ToString() == contactno)
    {
        ExistingnumberCount += 1;
        match = true;
        break;
    }
}

Context

StackExchange Code Review Q#150374, answer score: 41

Revisions (0)

No revisions yet.