2
\$\begingroup\$

Sometimes I need to get a single thing from a collection but I'm not very happy with the Single extension. It does not differentiate between empty and more than one element and throws the same InvalidOperationException in both cases. I find it's an important disadvantage and it helps a lot if you know excactly why and exception was thrown.


I want to replace it with my own extension that I currently name Single2 as throwing two different exceptions.

public static T Single2<T>(this IEnumerable<T> source)
{
    var count = 0;
    var single = default(T);
    using (var enumerator = source.GetEnumerator())
    {
        while (enumerator.MoveNext() && count++ < 1)
        {
            single = enumerator.Current;
        }
    }

    switch ((SearchResult)count)
    {
        case SearchResult.NotFound: throw new EmptySequenceException();
        case SearchResult.SingleMatch: return single;
        default: throw new MoreThanOneElementException();
    }
}

public enum SearchResult
{
    NotFound = 0,
    SingleMatch = 1,
    ManyMatches = 2,
}

The exceptions are very simple:

public class EmptySequenceException : Exception { }
public class MoreThanOneElementException : Exception { }

That's all. What do you think of this new helper? Is there still room for improvement?


Example

This is one of the methods that already uses the new Single2. It's from my ResourceReader and it searches for an embedded resource in an assembly.

public static string FindName<T>([NotNull] this IResourceReader resources, [NotNull] Expression<Func<string, bool>> predicateExpression)
{
    if (resources == null) throw new ArgumentNullException(nameof(resources));
    if (predicateExpression == null) throw new ArgumentNullException(nameof(predicateExpression));

    var predicate = predicateExpression.Compile();

    try
    {
        return
            resources
                .GetResourceNames(typeof(T).Assembly)
                .Where(predicate)
                .Single2();
    }
    catch (EmptySequenceException innerException)
    {
        throw DynamicException.Factory.CreateDynamicException(
            $"ResourceNotFound{nameof(Exception)}",
            $"Expression {predicateExpression.ToString().QuoteWith("'")} does not match any resource in the {typeof(T).Assembly.GetName().Name.QuoteWith("'")} assembly.",
            innerException);
    }
    catch (MoreThanOneElementException innerException)
    {
        throw DynamicException.Factory.CreateDynamicException(
            $"MoreThanOneResourceFound{nameof(Exception)}",
            $"Expression {predicateExpression.ToString().QuoteWith("'")} matches more than one resource in the {typeof(T).Assembly.GetName().Name.QuoteWith("'")} assembly.",
            innerException);
    }            
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Spelling fix: It's 'more than', not 'more then'. I think that enum actually degrades readability a bit (the exception and variable names already provide sufficient information, and it's not like 0 and 1 are 'magic values' in this context), but other than that it looks fine to me. \$\endgroup\$ – Pieter Witvoet Nov 15 '17 at 21:02
  • \$\begingroup\$ @PieterWitvoet Thx, it's my favourite mistake. Always using then instead of than, even though I actually know it's wrong, crap, I need to learn it once and for all :-] In the first version I actually had magic numbers but changed them first to const and eventually upgraded to enums. \$\endgroup\$ – t3chb0t Nov 15 '17 at 21:08
  • 1
    \$\begingroup\$ Oh, one more thing: using a custom base class for both exceptions would give you a little more flexibility: it allows you to also catch them together, for those situations where the distinction doesn't matter as much, without catching other kinds of exceptions. \$\endgroup\$ – Pieter Witvoet Nov 17 '17 at 8:22
  • \$\begingroup\$ @PieterWitvoet I'd give you +1 if you wrote this as an answer ;-) It's definitely a good idea. \$\endgroup\$ – t3chb0t Nov 17 '17 at 8:24
2
\$\begingroup\$

I don't really like the second while condition count++ < 1 because IMO it is to hard to grasp at first glance when this condition really comes true.

I would do it without a loop but with simple if statements like so

public static T Single2<T>(this IEnumerable<T> source)
{
    using (var enumerator = source.GetEnumerator())
    {
        if (enumerator.MoveNext() == false)
        {
            throw new EmptySequenceException();
        }
        var single = enumerator.Current;
        if (enumerator.MoveNext())
        {
            throw new MoreThanOneElementException();
        }
        return single;
    }
}  

Now you don't have a count anymore hence no switch either. I have not used the ! operator on purpose, because having two calls to MoveNext() in two different if condition one checking for false and the other for true harms the readability a little by using that operator.

\$\endgroup\$
  • \$\begingroup\$ This is a great suggestion ;-) and I'm on your side with the == false. It hurts readability here when you put ! in front of the condition. \$\endgroup\$ – t3chb0t Nov 16 '17 at 7:32

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.