DistinctCount extension method

Question

Here I go again. I have been finding a fairly common pattern in business logic code. And that pattern looks like this: int sprocketCount = datastore.GetSprocketOrders(parameters).Distinct().Count(); I decided I wanted to build DistinctCount() (again from "first principles") as Distinct() will create a second enumerable off of the first before Count() is executed. With that, here are four variations of DistinctCount():

public static int DistinctCount<TSource>(this IEnumerable<TSource> source) =>
    source?.DistinctCount((IEqualityComparer<TSource>)null) ?? throw new ArgumentNullException(nameof(source));

public static int DistinctCount<TSource>(this IEnumerable<TSource> source, IEqualityComparer<TSource> comparer)
{
    if (source is null)
    {
        throw new ArgumentNullException(nameof(source));
    }

    ISet<TSource> set = new HashSet<TSource>(comparer);
    int num = 0;

    using (IEnumerator<TSource> enumerator = source.GetEnumerator())
    {
        while (enumerator.MoveNext())
        {
            // ReSharper disable once AssignNullToNotNullAttribute
            if (set.Add(enumerator.Current))
            {
                checked
                {
                    ++num;
                }
            }
        }
    }

    return num;
}

public static int DistinctCount<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate)
{
    if (source is null)
    {
        throw new ArgumentNullException(nameof(source));
    }

    if (predicate is null)
    {
        throw new ArgumentNullException(nameof(predicate));
    }

    return source.DistinctCount(predicate, null);
}

public static int DistinctCount<TSource>(
    this IEnumerable<TSource> source,
    Func<TSource, bool> predicate,
    IEqualityComparer<TSource> comparer)
{
    if (source is null)
    {
        throw new ArgumentNullException(nameof(source));
    }

    if (predicate is null)
    {
        throw new ArgumentNullException(nameof(predicate));
    }

    ISet<TSource> set = new HashSet<TSource>(comparer);
    int num = 0;

    foreach (TSource source1 in source)
    {
        if (predicate(source1) && set.Add(source1))
        {
            checked
            {
                ++num;
            }
        }
    }

    return num;
}

And here are a battery of unit tests:

[TestMethod]
[ExpectedException(typeof(ArgumentNullException))]
public void TestNull()
{
    int[] nullArray = null;

    // ReSharper disable once ExpressionIsAlwaysNull
    Assert.AreEqual(0, nullArray.DistinctCount());
}

[TestMethod]
[ExpectedException(typeof(ArgumentNullException))]
public void TestNullPredicate()
{
    int[] zero = Array.Empty<int>();
    Func<int, bool> predicate = null;

    // ReSharper disable once ExpressionIsAlwaysNull
    Assert.AreEqual(0, zero.DistinctCount(predicate));
}

[TestMethod]
public void TestZero()
{
    int[] zero = Array.Empty<int>();

    Assert.AreEqual(0, zero.DistinctCount());
}

[TestMethod]
public void TestOne()
{
    int[] one = { 1 };

    Assert.AreEqual(1, one.DistinctCount());
}

[TestMethod]
public void TestOneWithDuplicate()
{
    int[] oneWithDuplicate = { 1, 1, 1, 1, 1 };

    Assert.AreEqual(1, oneWithDuplicate.DistinctCount());
}

[TestMethod]
public void TestTwo()
{
    int[] two = { 1, 2 };

    Assert.AreEqual(2, two.DistinctCount());
}

[TestMethod]
public void TestTwoWithDuplicate()
{
    int[] twoWithDuplicate = { 2, 1, 2, 1, 2, 2, 1, 2 };

    Assert.AreEqual(2, twoWithDuplicate.DistinctCount());
}

[TestMethod]
public void TestTwoWithDuplicateUsingPredicate()
{
    int[] twoWithDuplicate = { 2, 1, 3, 2, 1, 2, 2, 1, 2, 3 };

    Assert.AreEqual(2, twoWithDuplicate.DistinctCount(x => x > 1));
}

[TestMethod]
public void TestTwoUsingNullComparer()
{
    int[] two = { 1, 2 };
    IEqualityComparer<int> comparer = null;

    // ReSharper disable once ExpressionIsAlwaysNull
    Assert.AreEqual(2, two.DistinctCount(comparer));
}

[TestMethod]
public void TestOneWithDuplicateUsingComparer()
{
    string[] one = { "one", "One", "oNe", "ONE" };

    Assert.AreEqual(1, one.DistinctCount(StringComparer.InvariantCultureIgnoreCase));
}

[TestMethod]
public void TestTwoWithDuplicateUsingPredicateAndComparer()
{
    string[] two = { "one", "two", "One", "Two", "oNe", "TWO", "ONE", "tWo", "three" };

    Assert.AreEqual(2, two.DistinctCount(x => x != "three", StringComparer.InvariantCultureIgnoreCase));
}

As always, ooking for overall review - is the code readable, maintainable, performant? Do the tests have the right amount of coverage or are there more particular cases to consider?

Answer 1

---

As slepic in his comment, I also wonder why you use an enumerator in the first and foreach in the second place?

---

You can eliminate null checks in the versions that call other overrides:

public static int DistinctCount<TSource>(this IEnumerable<TSource> source) =>
        source?.DistinctCount((IEqualityComparer<TSource>)null) ?? throw new ArgumentNullException(nameof(source));

can be reduced to:

public static int DistinctCount<TSource>(this IEnumerable<TSource> source) => DistinctCount(source, (IEqualityComparer<TSource>)null);

And the other to:

public static int DistinctCount<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate) => DistinctCount(source, predicate, null);

---

Do you really need num? Couldn't you just return set.Count?

---

By using ToHashSet<T>() directly as show below I only find a minor loss (if any) in performance compared to your versions:

  public static class ExtensionsReview
  {
    public static int DistinctCount<TSource>(this IEnumerable<TSource> source) => DistinctCount(source, (IEqualityComparer<TSource>)null);

    public static int DistinctCount<TSource>(this IEnumerable<TSource> source, IEqualityComparer<TSource> comparer)
    {
      if (source is null)
      {
        throw new ArgumentNullException(nameof(source));
      }

      return source.ToHashSet(comparer).Count;
    }

    public static int DistinctCount<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate) => DistinctCount(source, predicate, null);

    public static int DistinctCount<TSource>(
        this IEnumerable<TSource> source,
        Func<TSource, bool> predicate,
        IEqualityComparer<TSource> comparer)
    {
      if (source is null)
      {
        throw new ArgumentNullException(nameof(source));
      }

      if (predicate is null)
      {
        throw new ArgumentNullException(nameof(predicate));
      }

      return source.Where(predicate).ToHashSet(comparer).Count;

    }
  }

---

According to your tests, I think you should test reference types (classes) with override of Equals()/GetHashCode() (and implementation of IEquatable<T>) with and without a custom comparer.

Answer 2

Slepic and Henrik are wondering about the use of foreach and enumerator, and I'm too.

Anyway, instead of having different versions with actual implementations for the same purpose (count the distinct elements), you can create one private method with the full implementation, and then, just call back this method on the other methods.

So, the main implementation would be like this :

private static int CountDistinctIterator<TSource>(IEnumerable<TSource> source, Func<TSource, bool> predicate, IEqualityComparer<TSource> comparer)
{
    if (source == null) throw new ArgumentNullException(nameof(source));

    var set = new HashSet<TSource>(comparer);
    var count = 0;
    foreach (TSource element in source)
    {
        checked
        {
            if (set.Add(element) && predicate(element))
            {
                count++;
            }

        }
    }

    return count;
}

Now, it's a matter of calling back this method with the appropriate arguments.

Like this :

public static int CountDistinct<TSource>(this IEnumerable<TSource> source)
{
    return CountDistinctIterator<TSource>(source, (s) => true, null);
}

public static int CountDistinct<TSource>(this IEnumerable<TSource> source,  IEqualityComparer<TSource> comparer)
{
    return CountDistinctIterator<TSource>(source, (s) => true, comparer);
}

public static bool AnyDistinct<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate)
{
    return CountDistinctIterator<TSource>(source, predicate, null) == 1;
}

public static bool AnyDistinct<TSource>(this IEnumerable<TSource> source)
{
    return CountDistinctIterator<TSource>(source, (s) => true, null) == 1;
}

although, for this Distinct I don't see any usage for Func<TSource, bool> predicate except for checking if the element exists or not. As the Distinct would get the unique elements, and if you say element == xxx it'll always return 1 if exists, and 0 if not. Unless there is any other uses except this one, in my opinion, I find it beneficial if rename this method:

DistinctCount<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate)

to something meaningful other than DistinctCount like for instance DistinctAny which return boolean (true if DistinctCount returns 1, false if 0).

UPDATE : I have changed the methods name from DistinctCount to CountDistinct the reason of this is because the method is Counting, so the Count needs to be first so it would be easier to picked up, the other reason is doing this will make it appear after Count on the intellisense list. I also added AnyDistinct methods which replaced the mentioned method (the one with Func<TSource, bool>).

Answer 3

Just looking at your tests, there's a couple of points to consider...

Naming

Having Test at the front of every test case is usually redundant (public methods in test classes are tests...). The beginning of the test name is also quite valuable real-estate since your test runner/window is likely to truncate what it displays after a certain number of characters. Consider removing the 'Test'. A better prefix might be the name of the method under test (although you may be using the name of the `TestClass for that, since you don't include that part of your code).

Make it clear what you're testing

I found your test methods that are testing for exceptions to be less than clear.

[TestMethod]
[ExpectedException(typeof(ArgumentNullException))]
public void TestNullPredicate()
{
    int[] zero = Array.Empty<int>();
    Func<int, bool> predicate = null;

    // ReSharper disable once ExpressionIsAlwaysNull
    Assert.AreEqual(0, zero.DistinctCount(predicate));
}

Initially I skipped over the method annotation and just rest the test code. On the face of it, it look like if there was a null predicate, you are expecting the method to return 0. This seemed odd, however possible behaviour. There's nothing in the test name (such as DistinctCount_NullPredicate_Throws) to indicate what the expected outcome was, then eventually there's the ExpectedException attribute, which explains that actually the test is expecting an ArgumentNullException.

Having an Assert statement when you're not actually expecting a value to be returned from the call is misleading. It would be better to just call the method (zero.DistinctCount(predicate)). The lack of an assertion helps to make it more obvious that the attributes indicate the success criteria for the test.