patterncsharpMajor
Country code lookup for each line in a CSV file
Viewed 0 times
fileeachlinecountrycsvforcodelookup
Problem
I'm trying to write a program that has a
Here's what the lambda expression looks like:
Same expression using Linq:
Here, the list
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();
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_CodeSame 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
Then suddenly we encounter
Property names should not contain an underscore:
Logic like "retrieve
You are using
Do you seriously expect
Why is there a comment saying
This:
can basically be replaced by
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
WRT your question: consider converting countries to a
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.