13
\$\begingroup\$

I have a method which compares two lists of strings. If the strings, and only the strings, from listA appear in listB, return true. Else return false.

internal bool DoIdsMatchThoseFromXml(List<string> Ids, List<string> XmlIds)
    {
        bool result = true;

        if (Ids.Count == XmlIds.Count)
        {
            foreach (var hwId in Ids)
            {
                if(!XmlIds.Contains(hwId))
                {
                    result = false;
                    break;
                }
            }
        }
        else
        {
            result = false;
        }

        return result;
    }

My initial unit test is passing (no edge cases etc. yet), but I was wondering if there is a more compact, but still readable, way to do it.

\$\endgroup\$
16
\$\begingroup\$

So let me get this straight, you are trying to determine if list A is a subset of list B?

internal bool DoIdsMatchThoseFromXml(List<string> ids, List<string> xmlIds)
{
    return !ids.Except(xmlIds).Any(); // A - B = {}
}

Or are you trying to see if list A is set equal to list B?

internal bool DoIdsMatchThoseFromXml(List<string> ids, List<string> xmlIds)
{
    return ids.Count == xmlIds.Count // assumes unique values in each list
        && new HashSet<string>(ids).SetEquals(xmlIds);
}

I would avoid performing linear searches through this as you'd be looking at O(n*m) performance when you could be doing O(n+m). Use set operations if you can. Use HashSet<string> if possible too.

\$\endgroup\$
  • \$\begingroup\$ Thanks Jeff. These (LINQ?) methods are new to me! Care to explain the xmlIds.Count > 0 in the second example? I'm not sure what this adds. \$\endgroup\$ – barry Jan 3 '12 at 21:14
  • \$\begingroup\$ In the second case, you need to make sure set B is non-empty otherwise it will always be true which would be incorrect. \$\endgroup\$ – Jeff Mercado Jan 3 '12 at 21:25
  • \$\begingroup\$ Hmm... with the non-empty check it is just the same as the first code example, except the lists swap places. So wouldn't the second example return whether B was a subset of A, just as the first example returns whether A is a subset of B? \$\endgroup\$ – barry Jan 3 '12 at 21:28
  • \$\begingroup\$ The way I see it, if you're checking if a set A is a subset of set B, just remove all common elements from set A that's also in set B (A - B) and it should yield the empty set. The empty set (as set A) is a subset of all sets so we need to allow for it. If you're checking if set A is equal to set B, then provided that set B is not empty, removing all from set B all in common from set A should yield the empty set. It's unlikely that both your lists will be empty so I've omitted that check (you can certainly add it if you wish). \$\endgroup\$ – Jeff Mercado Jan 3 '12 at 21:34
  • \$\begingroup\$ But in example 2, set B could just be a subset of set A. What if A = {1,2,3,4} and B = {1,2}? Apologies if I'm missing the obvious here! \$\endgroup\$ – barry Jan 3 '12 at 22:31
6
\$\begingroup\$

If you can use LINQ, I think you can do

internal bool DoIdsMatchThoseFromXml(List<string> Ids, List<string> XmlIds)
{
    return 
        Ids.Count == XmlIds.Count &&
        Ids.All(XmlIds.Contains) &&
        XmlIds.All(Ids.Contains);
}

to achieve the same result.

\$\endgroup\$
  • 2
    \$\begingroup\$ Perhaps Ids.All(XmlIds.Contains) might sound a bit Yoda-like, but I think it's still readable. \$\endgroup\$ – Cristian Lupascu Jan 3 '12 at 21:02
  • 2
    \$\begingroup\$ :-) Yoda like it does indeed sound \$\endgroup\$ – barry Jan 3 '12 at 21:18
4
\$\begingroup\$

This uses no LINQ, just methods defined on List<T>:

    internal static bool DoIdsMatchThoseFromXml(List<string> Ids, List<string> XmlIds)
    {
        return Ids.TrueForAll(XmlIds.Contains) && XmlIds.TrueForAll(Ids.Contains);
    }
\$\endgroup\$
  • \$\begingroup\$ I like how clean this one is. \$\endgroup\$ – Almo Jul 9 '14 at 21:54
2
\$\begingroup\$

The solutions proposed by Jeff and w0lf are very fine, but have you considered simply rearranging your code a little bit? Because even though the following does O(M*N), your question was about readability, not performance, and I think that people unfamiliar with linq will find this more graspable:

internal bool DoIdsMatchThoseFromXml(List<string> Ids, List<string> XmlIds)
{
    if (Ids.Count != XmlIds.Count)
        return false;
    foreach (var hwId in Ids)
        if(!XmlIds.Contains(hwId))
            return false;
    return true;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I'm no LINQ expert, but I found them eminently readable. LINQ is about describing intent (what) over implementation (how). Gory internals is why we've invented higher-level languages and libraries. \$\endgroup\$ – Jesse C. Slicer Jan 4 '12 at 2:02
  • \$\begingroup\$ I have to admit you are right. So, I will reword my answer a little bit. I am changing "people not awfully familiar with linq will find this more readable" to "people unfamiliar with linq will find this more graspable". \$\endgroup\$ – Mike Nakis Jan 4 '12 at 7:38
2
\$\begingroup\$

This is more compact, but also performs better:

internal bool DoIdsMatchThoseFromXml(List<string> Ids, List<string> XmlIds) {
  if (Ids.Count != XmlIds.Count) return false;
  HashSet<string> xmlIds = new HashSet<string>(XmlIds);
  return Ids.All(id => xmlIds.Contains(id));
}

This is close to O(n), compared to the original O(n*n).

\$\endgroup\$
1
\$\begingroup\$

If you sort the two lists and then return the SequenceEqual method you can do it all in three lines of code. SequenceEqual returns whether or not two Lists have the same items in the same order (hence the sorting before the comparing).

    internal bool DoIdsMatchThoseFromXml(List<string> Ids, List<string> XmlIds)
    {
        Ids.Sort();
        XmlIds.Sort();

        return Ids.SequenceEqual(XmlIds);
    }
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.