17
\$\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\$
8
  • 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 '19 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 '19 at 11:02
  • 2
    \$\begingroup\$ @BCdotWEB SingleOrDefault() throws an exception if there are multiple elements that fulfill the same criteria. \$\endgroup\$
    – M.Y. Babt
    Jul 24 '19 at 12:42
  • 1
    \$\begingroup\$ @MY_G True, but that's what you want IMHO. \$\endgroup\$
    – BCdotWEB
    Jul 24 '19 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 '19 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\$
18
  • 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 '19 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\$ Jul 23 '19 at 20:13
  • 4
    \$\begingroup\$ so is null means ReferenceEquals(null, source) then? I did not know this one \$\endgroup\$
    – dfhwze
    Jul 23 '19 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\$ Jul 23 '19 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 '19 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\$
9
  • 1
    \$\begingroup\$ Interesting alternative way of looking at the problem. You got my vote! \$\endgroup\$
    – dfhwze
    Jul 24 '19 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 '19 at 18:39
  • 1
    \$\begingroup\$ @dfhwze did you know that? SingleOrDefault is dangeours when used with a predicate :-\ \$\endgroup\$
    – t3chb0t
    Jul 24 '19 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\$ Jul 24 '19 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 '19 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\$
3
  • 1
    \$\begingroup\$ Seems as optimized as I can think of. Early exit is the best you can do. \$\endgroup\$
    – dfhwze
    Jul 23 '19 at 20:09
  • 1
    \$\begingroup\$ The predicate should be allowed to not be provided though, no? \$\endgroup\$ Jul 24 '19 at 5:22
  • 1
    \$\begingroup\$ @MathieuGuindon that version is what the original question has. Use them both together. \$\endgroup\$ Jul 24 '19 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.