7
\$\begingroup\$

I'm iterating through two lists to find matching objects. The objects have two string properties, Id1 and Id2 below. If Id1 is not null, empty or whitespace, I want to use it to find a matching object in the other list. Otherwise I want to make use of Id2.

If both Id1 and Id2 are set to null by the extension method on either p or p2, I want to skip the iteration, since I know that I won't find a match.

public static string NullIfEmptyOrWhiteSpace(this string value)
{
    return string.IsNullOrEmpty(value) ? null : value;
}
foreach (Product p2 in productList1)
{ 
    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.Id1.NullIfEmptyOrWhiteSpace() ??
                     p.Id2.NullIfEmptyOrWhiteSpace();

        string p2Id = p2.Id1.NullIfEmptyOrWhiteSpace() ??
                      p2.Id2.NullIfEmptyOrWhiteSpace();

        if (pId == null || p2Id == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

This seems to work, but it feels like I'm re-inventing the wheel here. Are there any built in methods that I can make use of to simplify the code? Perhaps a class that implements IEqualityComparer?

Example use case:

List<Product> list1 = new List<Product>()
{  
    new Product() { Id1 = "123", Id2 = null },
    new Product() { Id1 = "435", Id2 = null },
    new Product() { Id1 = null, Id2 = "123" }
}

List<Product> list12 = new List<Product>()
{  
    new Product() { Id1 = "123", Id2 = null },
    new Product() { Id1 = "456", Id2 = null },
    new Product() { Id1 = null, Id2 = "123" }
}

// do something with each matching item in the first list
// which would be item #1 and item #3 above
\$\endgroup\$
6
  • \$\begingroup\$ It isn't very clear to me what you're attempting to do but I feel like this can be greatly simplified with a few lines of LINQ. Can you give some sample input/output so I can add it to my answer? \$\endgroup\$ – Jeroen Vannevel Jul 1 '14 at 11:08
  • \$\begingroup\$ You can replace NullIfEmptyOrWhiteSpace with the built-in method string.IsNullOrWhiteSpace (although you'll have to change the logic around a little). As it is right now it's hard to follow what you're trying to do. \$\endgroup\$ – Jeroen Vannevel Jul 1 '14 at 11:20
  • \$\begingroup\$ @JeroenVannevel Does my update help? \$\endgroup\$ – Johan Jul 1 '14 at 11:23
  • \$\begingroup\$ Just noticed that your question mentions two lists, but your code only uses one (products). \$\endgroup\$ – mjolka Jul 1 '14 at 12:10
  • \$\begingroup\$ @mjolka Woops, typo. Sorry - I'll update \$\endgroup\$ – Johan Jul 1 '14 at 12:11
6
\$\begingroup\$

Let's start refactoring!

foreach (Product p2 in productList1)
{ 
    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.Id1.NullIfEmptyOrWhiteSpace() ??
                     p.Id2.NullIfEmptyOrWhiteSpace();

        string p2Id = p2.Id1.NullIfEmptyOrWhiteSpace() ??
                      p2.Id2.NullIfEmptyOrWhiteSpace();

        if (pId == null || p2Id == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

p2Id does not depend on p, so let's move it out of the lambda.

foreach (Product p2 in productList1)
{ 
    string p2Id = p2.Id1.NullIfEmptyOrWhiteSpace() ??
                  p2.Id2.NullIfEmptyOrWhiteSpace();

    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.Id1.NullIfEmptyOrWhiteSpace() ??
                     p.Id2.NullIfEmptyOrWhiteSpace();

        if (pId == null || p2Id == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

Now let's define a method on Product to remove the repetition. I called it GetKey, but there's probably a better name.

public string GetKey()
{
    return this.Id1.NullIfEmptyOrWhiteSpace() ?? this.Id2.NullIfEmptyOrWhiteSpace();
}

foreach (Product p2 in productList1)
{ 
    string p2Id = p2.GetKey();

    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.GetKey();

        if (pId == null || p2Id == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

It looks like you actually want to call IsNullOrWhiteSpace in NullIfEmptyOrWhiteSpace, so let's fix that.

public static string NullIfEmptyOrWhiteSpace(this string value)
{
    return string.IsNullOrWhiteSpace(value) ? null : value;
}

Actually, we don't need to go through productList2 if p2Id is null.

foreach (Product p2 in productList1)
{ 
    string p2Id = p2.GetKey();
    if (p2Id == null)
    {
        continue;
    }

    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.GetKey();

        if (pId == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

Given that we only use p2 for its key, we can rewrite this as

foreach (var key in productList1.Select(product => product.GetKey()).Where(key => key != null))
{
    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.GetKey();

        if (pId == null)
            return false;

        return pId.Equals(key);
    });

    // do stuff with match
}

Now let's use a dictionary to replace the call to productList2.FirstOrDefault.

var firstProductWithKey = new Dictionary<string, Product>();
foreach (var product in productList2)
{
    var key = product.GetKey();
    if (key != null && !firstProductWithKey.ContainsKey(key))
    {
        firstProductWithKey.Add(key, product);
    }
}

if (!firstProductWithKey.Any())
{
    return;
}

foreach (var key in productList1.Select(product => product.GetKey()).Where(key => key != null))
{
    Product match;
    if (firstProductWithKey.TryGetValue(key, out match))
    {
        // do stuff with match
    }
}

So what have we gained? Hopefully the intention of the code is a bit clearer, but there's also a performance benefit.

Let's say that the lists have \$n\$ and \$m\$ elements respectively, and that there are no keys common to the two lists.

In the original code, we're evaluating each key in the first list \$m\$ times, and each key in the second list \$n\$ times. So \$2nm\$ keys in total.

In the refactored version, we evaluate each key in the first list once, and each key in the second list once, for \$n + m\$ keys. We're performing much less work, and generating much less garbage for the GC.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I think the key here (no pun intended) is the Key property. What do you think about a custom Equals method (or similar) as a method on the Product, that uses the new Key? Thanks for the input! \$\endgroup\$ – Johan Jul 1 '14 at 11:56
  • 1
    \$\begingroup\$ You can make a custom method, but don't call it Equals. Save that for when you really need to compare items for complete equality. I think it would be better to create a new interface and implement it. But ask yourself: What does a custom method get you beyond just comparing the property? \$\endgroup\$ – Snowbody Jul 1 '14 at 12:54
  • \$\begingroup\$ @Snowbody Okay. I figured that it might be handy if I extend the id check later on, by let's say, introducing a third property. Then I can update the logic in one common place. \$\endgroup\$ – Johan Jul 1 '14 at 13:18

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.