3
\$\begingroup\$

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);                    
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ 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. \$\endgroup\$
    – user24822
    Feb 28 '14 at 16:27
  • \$\begingroup\$ What's _divider? \$\endgroup\$ Feb 28 '14 at 16:55
  • 2
    \$\begingroup\$ You are never using _data, and you did not provide a sample of _divider[1]. Please review and update your question. \$\endgroup\$
    – ANeves
    Feb 28 '14 at 17:32
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Feb 28 '14 at 23:34
5
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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: https://stackoverflow.com/a/22100355/148412

\$\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.