LINQ THINK

The more I use LINQ, the more I run into legacy code that can easily be improved. The nice thing about LINQ is that it can make your code more elegant and easier to understand, as well as, in most cases, more efficient.

Let’s take a simple  example. Here I’ve extracted some legacy code in the form of a method that tells you if a given string “Key” is present in a string array. First, the “OLD” way:
//"OLD" way:
static bool IsKeyInArray(string[] items, string key)
{
if (items == null) return false;
if (items.Length < 1) return false;
if (String.IsNullOrEmpty(key)) return false;
foreach (string item in items)
{
if (item.Trim() == key)
{
return true;
}
}
return false;
}


The obvious  thing to see here is that we have to iterate over the entire array. Now the new “LINQY” way:


static bool IsKeyInArray2(string[] items, string key)
{
bool isInArray = false;
if (items == null || items.Length <1 || String.IsNullOrEmpty( key)) 
return false;
if (items.ToList().Contains(key))
isInArray = true;
return isInArray;
}


Note that by simply calling the ToList() extension method, we’ve automatically converted our string array into a “LINQY” construct. No iteration needed, just use the “Contains( key)” method.

Comments

  1. You could make it even more simple and more specific about code's intent.

    static bool IsKeyInArray3(string[] items, string key) {
    if (string.IsNullOrEmpty(key) || items == null) return false;

    return strings.Any(s => s.Trim() == key);
    }

    ReplyDelete
  2. I'd get rid of the flag variable and take advantage of the order of the if statement. Then you can write it all on one line - like so..

    return items != null
    && items.Length >0
    && !String.IsNullOrEmpty( key)
    && items.ToList().Contains(key);

    Or if thats not clear you could split into 2 statements as you've done. The parameter check first, then the Linq statement.

    ReplyDelete
  3. I agree on the use of LINQ as a tremendous boon for improvement of legacy code: the readability of a LINQ query is tremendous, and you can remove a massive amount of code tying LINQ in with Lamdas.

    One item, you can further refine your algorithm:

    static bool IsKeyInArray2(string[] items, string key)
    {
    if (items == null || items.Length <1 || String.IsNullOrEmpty(key))
    return false;

    return items.ToList().Contains(key);
    }

    This would provide you the same functionality without requiring a variable with an insanely short lifetime. Not really a big deal, just another level of simplification.

    ReplyDelete
  4. by using .ToList() you are not LINQ-ifying the array but instead converting the whole array to a generic List and using the List.Contains(T) method. This will result in much worse performance as new memory allocation are needed.

    Also the original code uses .Trim() to compare the keys and the new one does not.

    The correct way to use LINQ in this case would be
    IsKeyInArray(string[] items, string key)
    {
    if (items == null) return;
    key = key == null ? key : key.Trim();
    return items.Any(o => string.Equals(o, key, StringComparison.Ordinal));
    }

    But if you have the method, leave it as is to get better performance. If you choose to use LINQ approach, delete this method and use LINQ expressions directly.

    ReplyDelete
  5. Since string[] already implements IEnumerable, you don't need the ToList() method. Also, instead of using .Contains, you may want to consider .Any which can take a predicate and thus has more flexibility in cases where Contains isn't natively supported by the underlying data source like the Dictionary. In your case, you could replace the contains line with this:


    isInArray = items.Any(item => item == key));

    ReplyDelete
  6. Anonymous10:29 AM

    Why the need to use "ToList()"? You can use Contains on an Array.

    Also, I prefer this kind of syntax for simple boolean methods:

    bool IsInArray(string[] items, string key)
    {
    return
    items == null
    ||
    items.Length < 1
    ||
    string.IsNullOrEmpty(key)
    ?
    false
    :
    items.Contains(key);
    }
    //With proper indentation I can't do on this blog ;)

    ReplyDelete
  7. This is one of those "before coffee" posts where the comments are more valuable than the original post. Thanks all.

    ReplyDelete
  8. ok, I was going to comment; you might want to update the post

    ReplyDelete
  9. @BlackTigerX,
    No need to update the post. The expert commenters have provided a rich set of information that tells all.

    ReplyDelete
  10. Anonymous8:28 AM

    good post Peter and great comments everyone - thanks

    ReplyDelete

Post a Comment

Popular posts from this blog

FIREFOX / IE Word-Wrap, Word-Break, TABLES FIX

Some observations on Script Callbacks, "AJAX", "ATLAS" "AHAB" and where it's all going.

IE7 - Vista: "Internet Explorer has stopped Working"