Having Single and SingleOrDefault to make a more succinct exception

Question

When calling Single or SingleOrDefault on an IEnumerable<T> and has more than one result, it throws an InvalidOperationException .

While the actual exception message is very descriptive, it's hard to write a catch that will only handle cases where calls to Single / SingleOrDefault fail.

 public virtual Fee GetFeeByPromoCode(string promoCode) { try { return _fees.SingleOrDefault(f => f.IsPromoCodeValid(promoCode)); } catch (InvalidOperationException) { throw new TooManyFeesException(); } } 

In this case, if IsPromoCodeValid also throws an InvalidOperationException , then it becomes ambiguous as to what catch is handling.

I could check the exception message, but I would like to avoid it, because I think it is dirty to process the code depending on the exception message.

My current alternative to SingleOrDefault as follows:

 public virtual Fee GetFeeByPromoCode(string promoCode) { var fees = _fees.Where(f => f.IsPromoCodeValid(promoCode)).ToList(); if (fees.Count > 1) { throw new InvalidFeeSetupException(); } return fees.FirstOrDefault(); } 

However, this code is much less obvious than the code above, and it also creates a less efficient query (if you use ORM with linq support) than using SingleOrDefault .

I could also do Take(2) with my second example to optimize it a bit, but this further confuses the purpose of the code.

Is there a way to do this without writing my own extension for both IEnumerable and IQueryable ?

Answer 1

Could this solve the problem?

 public virtual Fee GetFeeByPromoCode(string promoCode) { try { return _fees.SingleOrDefault(f => { try { return f.IsPromoCodeValid(promoCode); } catch(InvalidOperationException) { throw new PromoCodeException(); } }); } catch (InvalidOperationException) { throw new TooManyFeesException(); } } 

Answer 2

InvalidOperationException is pretty general. Any of the available properties (or even deeper on the stack) may cause this exception. Therefore, one way is to use your own method of exclusion and extension. For instance:

 static class EnumerableExtensions { public static TSource ExactlyOneOrZero<TSource>( this IEnumerable<TSource> source, Func<TSource, bool> predicate) { if (source == null) { throw new ArgumentNullException("source"); } if (predicate == null) { throw new ArgumentNullException("predicate"); } IEnumerable<TSource> matchingItems = source.Where(predicate); IReadOnlyList<TSource> limitedMatchingItems = matchingItems.Take(2).ToList(); int matchedItemCount = limitedMatchingItems.Count; switch (matchedItemCount) { case 0: return default(TSource); case 1: return limitedMatchingItems[0]; // Or Single() default: throw new TooManyMatchesException(); } } } class TooManyMatchesException : Exception { /* Don't forget to implement this properly. */ } 

This keeps the source code clean:

  public virtual Fee GetFeeByPromoCode(string promoCode) { try { return _fees.ExactlyOneOrZero(f => f.IsPromoCodeValid(promoCode)); } catch (TooManyMatchesException) { throw new TooManyFeesException(); } } 

Another way to do this is to use TryGet... -pattern, but it's not very clean. TryGetSingle will return true even if there are no matching elements. You can replace the boolean value with enum (Valid / Invalid), but I will leave it to the reader whether it is readable or not.

Answer 3

I consider First () / Single () / SingleOrDefault () as a kind of Assert.

i.e. If you use them, you will not want to catch an exception. Something is very wrong with your data, and it should be considered a critical error.

If your model has multiple results, do not use exceptions to verify.

From this point of view, I do not think your version of Take (2) is less obvious.