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);
}
}
1 Answer 1
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.
-
\$\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– t3chb0t2017年11月16日 07:32:23 +00:00Commented Nov 16, 2017 at 7:32
Explore related questions
See similar questions with these tags.
const
and eventually upgraded to enums. \$\endgroup\$