16
\$\begingroup\$

I often find in codebases something on the order of if (sprockets.Count() > 0) which is easily replaced with LINQ's if (sprockets.Any()). This keeps the entirety of sprockets from having to be iterated over completely (to get the count) then comparing to zero. Further, the business logic often reads something like "if there are any sprockets, inform the user of the subtotal". I also often see similar logic for exactly one of something: if (sprockets.Count() == 1) which doesn't have an easy, low-cost LINQ alternative. So I've created one here:

public static bool ExactlyOne<TSource>(this IEnumerable<TSource> source)
{
    if (source is null)
    {
        throw new ArgumentNullException(nameof(source));
    }

    using (IEnumerator<TSource> enumerator = source.GetEnumerator())
    {
        return enumerator.MoveNext() && !enumerator.MoveNext();
    }
}

Usage is if (sprockets.ExactlyOne()) Here are unit tests. There is one helper method called Infinite() which is a never-ending enumerable, which will baffle sprockets.Count(), but not sprockets.ExactlyOne():

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

        Assert.IsFalse(nullArray.ExactlyOne());
    }

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

        Assert.IsFalse(zero.ExactlyOne());
    }

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

        Assert.IsTrue(one.ExactlyOne());
    }

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

        Assert.IsFalse(two.ExactlyOne());
    }

    [TestMethod]
    public void TestInfinite()
    {
        IEnumerable<int> infinite = Infinite();

        Assert.IsFalse(infinite.ExactlyOne());
    }

    private static IEnumerable<int> Infinite()
    {
        while (true)
        {
            yield return 0;
        }
    }
}

Looking for overall review - is the code readable, maintainable, performant. Do the tests cover the expected cases or are there more to consider?

\$\endgroup\$
  • 2
    \$\begingroup\$ This discards the element. Have you considered designs that preserve the element, but also tell you if it's the only one? \$\endgroup\$ – Alexander Jul 24 at 5:55
  • 5
    \$\begingroup\$ Wouldn't it be better to use SingleOrDefault() in such cases? Or Single() if there has to be a value? \$\endgroup\$ – BCdotWEB Jul 24 at 11:02
  • 2
    \$\begingroup\$ @BCdotWEB SingleOrDefault() throws an exception if there are multiple elements that fulfill the same criteria. \$\endgroup\$ – MY_G Jul 24 at 12:42
  • 1
    \$\begingroup\$ @MY_G True, but that's what you want IMHO. \$\endgroup\$ – BCdotWEB Jul 24 at 13:46
  • 4
    \$\begingroup\$ Just as a reference F# has an implementation of ExactlyOne and it's pretty much what you have: github.com/fsharp/fsharp/blob/… \$\endgroup\$ – Ringil Jul 24 at 14:36
15
\$\begingroup\$

Q&A

Is the code readable?

  • ExactlyOne states very clearly what the method is supposed to do.
  • source is null seems odd to me (does that even compile?). I'd prefer source == null. (Edit from comments: a topic about is null vs == null)
  • IEnumerator<TSource> enumerator = source.GetEnumerator() can be written as var enumerator = source.GetEnumerator().

Is the code maintainable?

  • Since you are looking for a sibling function of Any<T>(), I would also include a ExactlyOne<T>(Func<T, bool> predicate).

Is the code performant?

  • It does seem so, right? But notice that LINQ is optimized for IEnumerable<T> that is also ICollection<T>, in which case Count is used. Implementations should have an eager implementation of this property. Your method could also use this optimization Count == 1.
  • I actually noticed (in my eyes) unexpected behavior in LINQ: Count<T>() is optimized for ICollection<T> but Any<T() is not. This means you arguably could make a slightly faster implementation than LINQ.
  • An in-depth comparison is required to find the most optimized approach. As discussed in the comments, testing the different implementations against a variety of input sources should yield us which method perfoms best under which conditions.

Do the tests cover the expected cases or are there more to consider?

You cover null, empty, 1, multiple, early exit on infinite.. but perhaps also test on ICollection<T> and custom IEnumerable<T> implementations with eager and/or lazy loading.


Reference Source: LINQ Any vs Count

// not optimized for ICollection<T> (why ??)
public static bool Any<TSource>(this IEnumerable<TSource> source) {
    if (source == null) throw Error.ArgumentNull("source");
    using (IEnumerator<TSource> e = source.GetEnumerator()) {
        if (e.MoveNext()) return true;
    }
    return false;
}

public static bool Any<TSource>(this IEnumerable<TSource> source, Func<TSource, bool> predicate) {
    if (source == null) throw Error.ArgumentNull("source");
    if (predicate == null) throw Error.ArgumentNull("predicate");
    foreach (TSource element in source) {
        if (predicate(element)) return true;
    }
    return false;
}

// optimized for ICollection<T>
public static int Count<TSource>(this IEnumerable<TSource> source) {
    if (source == null) throw Error.ArgumentNull("source");
    ICollection<TSource> collectionoft = source as ICollection<TSource>;
    if (collectionoft != null) return collectionoft.Count;
    ICollection collection = source as ICollection;
    if (collection != null) return collection.Count;
    int count = 0;
    using (IEnumerator<TSource> e = source.GetEnumerator()) {
        checked {
            while (e.MoveNext()) count++;
        }
    }
    return count;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Good thinking, it might very well be a complete clone of that method. On the other hand, I think it can be optimized with early exit when count > 1 \$\endgroup\$ – dfhwze Jul 23 at 20:03
  • 2
    \$\begingroup\$ source is null - I read about that as well as source is object (sorta like source != null) on some twitter feed from a noted developer, but the gist of it can be found here: gullberg.tk/blog/is-null-versus-null-in-c \$\endgroup\$ – Jesse C. Slicer Jul 23 at 20:13
  • 4
    \$\begingroup\$ so is null means ReferenceEquals(null, source) then? I did not know this one \$\endgroup\$ – dfhwze Jul 23 at 20:15
  • 1
    \$\begingroup\$ And I just added your suggested ICollection optimization using C#7 pattern matching: if (source is ICollection<TSource> collection) return collection.Count == 1; before the using. Thanks! \$\endgroup\$ – Jesse C. Slicer Jul 23 at 20:17
  • 2
    \$\begingroup\$ @Robert the null check is consistent with the API of Linq. Fail early is never a bad thing. \$\endgroup\$ – JAD Jul 25 at 6:52
12
\$\begingroup\$

mhmmm... I'm not so sure about this implementation. @dfhwze's points are valid but still, I find it's overengineered.

I prefer chaining these two alraedy available extensions that can do all mentioned tricks:

return source.Take(2).Count() == 1

or with a predicate

return source.Where(predicate).Take(2).Count() == 1;
\$\endgroup\$
  • 1
    \$\begingroup\$ Interesting alternative way of looking at the problem. You got my vote! \$\endgroup\$ – dfhwze Jul 24 at 7:15
  • 3
    \$\begingroup\$ @JesseC.Slicer SingleOrDefault with a predicate is even so similar to mine that they also use a swith. ok, but their solution is really stupid, it'll enumerate the entire collection! :-o I'm shocked: new [] { 1, 2, 3, 4}.Select(x => x.Dump()).SingleOrDefault(n => true).Dump(); the inner Dump will output everything. \$\endgroup\$ – t3chb0t Jul 24 at 18:39
  • 1
    \$\begingroup\$ @dfhwze did you know that? SingleOrDefault is dangeours when used with a predicate :-\ \$\endgroup\$ – t3chb0t Jul 24 at 18:45
  • 2
    \$\begingroup\$ @t3chb0t the scary part is, there's likely code dependent on the suboptimal behavior (side effects) so they'll not rework it properly. \$\endgroup\$ – Jesse C. Slicer Jul 24 at 19:52
  • 2
    \$\begingroup\$ @JesseC.Slicer: Exactly. It's been fixed in .net core, but they are hesitant to backport it to the Framework for the exact reason that you mentioned. See github.com/dotnet/corefx/issues/25079 for details. \$\endgroup\$ – Heinzi Jul 24 at 20:50
11
\$\begingroup\$

This is for dfhwze as per comment:

public static bool ExactlyOne<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));
    }

    bool gotOne = false;

    foreach (TSource element in source)
    {
        if (!predicate(element))
        {
            continue;
        }

        if (gotOne)
        {
            return false;
        }

        gotOne = true;
    }

    return gotOne;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Seems as optimized as I can think of. Early exit is the best you can do. \$\endgroup\$ – dfhwze Jul 23 at 20:09
  • 1
    \$\begingroup\$ The predicate should be allowed to not be provided though, no? \$\endgroup\$ – Mathieu Guindon Jul 24 at 5:22
  • 1
    \$\begingroup\$ @MathieuGuindon that version is what the original question has. Use them both together. \$\endgroup\$ – Jesse C. Slicer Jul 24 at 14:20

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.