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 the following key value pair in an array, and am trying to extract and load them into a collection.

The below code is working but it can be optimized using Linq:

string _data = "Website=url:www.site1.com,isdefault:true,url:www.site2.com,isdefault:true";

List<WebSiteAddress> _websiteList = new List<WebSiteAddress>() ;
WebSiteAddress _website = new WebSiteAddress();

string[] _websiteData = _divider[1].Split('=');
string[] _WebsiteKeyValuePair = _websiteData[1].Split(',');

for (int j = 0; j < _WebsiteKeyValuePair.Length; j++)
{
    string key = _WebsiteKeyValuePair[j].Split(':')[0];
    string value = _WebsiteKeyValuePair[j].Split(':')[1];

    if (key.ToLower() == "url")
    {
        _initWebsite.Url = value;
    }
    else if (key.ToLower() == "isdefault")
    {
        _website.IsDefault = Convert.ToBoolean(value);
        _websiteList.Add(_website);                    
    }
}
share|improve this question
    
There's some discussion about where this belongs. Someone (not the OP) migrated/copied it here from StackOverflow. It's got at least one answer on SO. –  Grant Winney Feb 28 at 16:27
    
What's _divider? –  Mat's Mug Feb 28 at 16:55
2  
You are never using _data, and you did not provide a sample of _divider[1]. Please review and update your question. –  ANeves Feb 28 at 17:32
1  
Instead of performing the same split operation twice to get the key and value you should split once and store the result in a temporary variable first. You should also do the ToLower() operation when you assign the value to key. That way you don't have to do a ToLower() in each if statement. –  David DeMar Feb 28 at 23:34
add comment

2 Answers 2

Naming

I don't get the _ underscore prefix. This typically identifies a private field (albeit controversial). I'm pretty sure there's a consensus about simple camelCasing for local identifiers.

The name _websiteList is a bad one, for two reasons:

  • It's of type WebSiteAddress, which has words Web and Site; the name isn't following the established casing convention, should be _webSiteList.
  • It's tying the variable to its type. Would the name still make sense if the type was WebSiteAddress[]?

A better name would be, simply, webSiteAddresses.

Typing

If you want to use LINQ, you're targeting .NET 3.5+. Therefore, you can use implicit typing to improve the readability (and possibly the maintainability) of your code (although that's possibly personal preference).

Consider:

  • var webSiteData = _divider[1].Split('=');

The SO answer gives you a nice LINQ-oriented approach.

share|improve this answer
add comment

My suggestions:

  • Use a foreach instead of a for;
  • Don't redo operations such as _WebsiteKeyValuePair[j].Split(':') and key.ToLower(), store them in a variable instead;
  • Use a switch instead of an if-chain.

Switch would look like:

switch(key.ToLower())
{
    case "url":
        _initWebsite.Url = value;
        break;
    case "isdefault":
        ...
        break;
}

As for LINQ, you have an answer in the Stackoverflow cross-post: http://stackoverflow.com/a/22100355/148412

share|improve this answer
add comment

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.