38

What would you prefer to see?

try
{
  var item = list.Single(x => x.HasFoo);
}
catch(InvalidOperationException e)
{
  throw new InvalidOperationException("Exactly one item with foo expected, none found", e);
}

Or:

var item = list.SingleOrDefault(x => x.HasFoo);
if (item == null)
      throw new InvalidOperationException("Exactly one item with foo expected, none found");

What's the best practice here? Which one makes the exception more comprehensible?

abatishchev
  • 98,240
  • 88
  • 296
  • 433
the_drow
  • 18,571
  • 25
  • 126
  • 193
  • I prefer second, because its compact – Stecya Apr 11 '11 at 09:30
  • 5
    SingleOrDefault will still throw exception if more than one item though.. – Richard Friend Apr 11 '11 at 09:36
  • 14
    I don't see the point of the try/catch block around the first option. You're not doing anything with the exception, all you're doing is throwing another exception of the same type with a different message. – forsvarir Apr 11 '11 at 09:37
  • Note that the first one feels like you are using the exception handler (which is expensive) to process logic which is not a great practice, although to be fair the second then throws an exception - so is it perhaps better to handle the exception in the second rather than throw a new one? – Mark Schultheiss Jan 31 '17 at 21:38
  • 1
    @forsvarir: It is often very useful to add context-dependent information like "with foo". The default exception reports "Sequence contains no elements" or "Sequence contains more than one element" which impart no information about which sequence or why we expect exactly one element. Imagine this is part of a large project, say a job scheduler that gracefully handles exceptions in any one given job. Ideally you should be able to diagnose errors from the log message alone, without having to follow a stack trace through multiple levels of architecture, source revisions, and/or InternalExceptions. – Tim Sparkles Mar 07 '17 at 20:31

9 Answers9

86
  • Use SingleOrDefault() if 0 or 1 items are expected
  • Use Single() if 1, not 0 or 2 and more, item is expected

Also keep in mind that there are a number of possible scenarios:

  • You got 0 when 0 or 1 was expected (ok)
  • You got 1 when 0 or 1 was expected (ok)
  • You got 2 or more when 0 or 1 was expected (error)

And:

  • You got 0 when 1 was expected (error)
  • You got 1 when 1 was expected (ok)
  • You got 2 or more when 1 was expected (error)

And don't forget about First(), FirstOrDefault() and Any()

šljaker
  • 7,294
  • 14
  • 46
  • 80
abatishchev
  • 98,240
  • 88
  • 296
  • 433
  • 5
    Just adding a comment because this answer alone does not point out that the exception code in the question is wrong; in the first snippet the exception should read, "Exactly one item with foo expected, zero or more than one were found" – Kieren Johnstone Apr 11 '11 at 10:21
  • @abatishchev, and what about if we want to get list of items, 0 or ~(many) items are expected, then what should we use? – Roxy'Pro Oct 05 '16 at 19:11
  • @Roxy'Pro: can you please elaborate your question? Or ask a new one and post here a link to it. – abatishchev Oct 06 '16 at 18:49
5

I would write:

var item = list.Single(x => x.HasFoo);

If the case where this does not return a single item is so common you need a friendlier error message, then is it really an exception at all?

Myster
  • 17,704
  • 13
  • 64
  • 93
4

Practically, they are the same. But I prefer second one since one exception is thrown while in the first two. Exceptions are expensive.

Aliostad
  • 80,612
  • 21
  • 160
  • 208
  • 4
    Since this is a "You have a bug" type exception performance is most likely no issue in the error case. – CodesInChaos Apr 11 '11 at 09:49
  • @CodeInChaos: It is a valid point though. What if this is a valid use case that could be handled when the exception is caught? – the_drow Apr 11 '11 at 10:05
  • 2
    On that note, exceptions should remain exceptional: if you're expecting it, and it's a valid use case, don't ask for exceptions to be thrown and don't throw them – Kieren Johnstone Apr 11 '11 at 10:08
  • 1
    @KierenJohnstone: You know, python programmers would disagree. Why is this such an issue in C#? – the_drow Apr 11 '11 at 10:16
  • @the_drow - I don't know the reason, but the saying goes that in C#, exceptions are exceptional; however see discussion at http://stackoverflow.com/questions/180937/are-exceptions-really-for-exceptional-errors – Kieren Johnstone Apr 11 '11 at 10:18
  • 1
    (My argument would be that they are expensive - things like stack traces, logging, are geared around errors and debugging, not passing a message to another part of the app) – Kieren Johnstone Apr 11 '11 at 10:22
  • If it were an exception you expect to be caught you wouldn't use `InvalidOperationException`. There are exceptions which should be caught, such as `IOException`. – CodesInChaos Apr 11 '11 at 10:36
  • @CodeInChaos: There are cases where an InvalidOperationException could be handled. In this case for example setting the HasFoo for one of the elements if it makes any sense and calling the function again. – the_drow Apr 11 '11 at 11:15
  • 2
    In that case you should avoid using `InvalidOperationException` and `Single` since using them states: I'm sure this won't happen. – CodesInChaos Apr 11 '11 at 11:18
4

I think it's OK to write

var item = list.SingleOrDefault(x => x.HasFoo);
if (item == null) ...

but you can also write

if (list.Any(x => x.HasFoo)) ...

if you don't actually need access to the value.

Roy Dictus
  • 32,551
  • 8
  • 60
  • 76
2

If you ALWAYS EXPECT  one element in the list, just use 

var item = list.Single(x => x.HasFoo);

and catch exception at the top level method, where you will log details of exception and show friendly message to the user.

If you sometimes expect 0 or more than 1 elements, the safest method will be

var item = list.FirstOrDefault(x => x.HasFoo);
if (item == null) 
{ 
// empty list processing, not necessary throwing exception
}

I assumed, that it is not important to verify, are more than 1 record exist or not.

Similar question was discussed in Code Project article LINQ: Single vs. SingleOrDefault

Michael Freidgeim
  • 26,542
  • 16
  • 152
  • 170
1

Assuming you were asking about the 0..1 scenario, I prefer SingleOrDefault because it lets you specify your own way to handle the "nothing found" scenario.

So, a good way to do using a little syntactic sugar, would be:

// assuming list is List<Bar>();
var item = list.SingleOrDefault(x => x.HasFoo) ?? notFound<Bar>();

where notFound() is:

T notFound<T>()
{
  throw new InvalidOperationException("Exactly one item with foo expected, none found");
}
Juanjo
  • 353
  • 1
  • 7
1

I'd rather see a check to the number of elements in the list before getting the element, rather than waiting for an exception, then throwing a new one.

var listFiltered = list.Where(x => x.HasFoo).ToList();
int listSize = listFiltered.Count();
if (listSize == 0)
{
    throw new InvalidOperationException("Exactly one item with foo expected, none found");
}
else if (listSize > 1)
{
    throw new InvalidOperationException("Exactly one item with foo expected, more than one found");
}

It's nice that the suggestions are compact, but better to be more explicit IMO.

(Also in your suggestions the exceptions are not strictly valid: they say 'none found' when there could be more than one)

Edit: Jeebus, added one line to filter the list first for pedantic people. (I thought it would have been obvious for anyone)

Kieren Johnstone
  • 41,277
  • 16
  • 94
  • 144
  • 5
    Doesn't answer the question: the OP is calling `Single`/`SingleOrDefault` with a predicate. Besides, not all sequences allow multiple enumerations, in which case you wouldn't be able to retrieve the desired item once you'd performed a `Count`. (Not to mention that your technique might need two passes through the sequence -- once for `Count`, once for getting the item -- when there are perfectly good built-in constructs that can do it in one pass.) – LukeH Apr 11 '11 at 09:33
  • "Would you prefer to see Single or SingleWithDefault" -> pointed out that I would prefer a different option and corrected an error in the Exception. Did answer the question ? – Kieren Johnstone Apr 11 '11 at 09:34
  • The questioner is using the version of `Single`/`SingleOrDefault` that takes a predicate. The number of elements in the whole list isn't relevant; what matters is how many satisfy `HasFoo`. – Gareth McCaughan Apr 11 '11 at 09:34
  • You could first filter the list and then check its number of elements, of course. That's certainly more explicit than using `Single` or `SingleOrDefault`; whether it's better is another matter. – Gareth McCaughan Apr 11 '11 at 09:35
  • 2
    After this you'll have to use the `.First` or `.Single` in anycase to get the element. It could be that the list then get enumerated twice in stead of only the once because of the deferred execution of the query (the enumeration will execute on the `.Count` and then again on the `.First` except, except for some types like array's and `List`, but you don't know the type in this case and can only assume `IEnumerable`) – Cornelius Apr 11 '11 at 09:52
  • I disagree here. Single is a perfectly valid construct that is built into .NET – the_drow Apr 11 '11 at 09:56
  • You guys are missing the point, the list is filtered first - ONE enumeration takes place. The question was about `Single` vs `SingleOrDefault` - to which I reply, 'neither is explicit enough for my tastes' – Kieren Johnstone Apr 11 '11 at 10:08
  • 2
    @Kieren One enumeration happens on `.Count()` and one happens when you finally extract the element with `.First()` or `.Single()`. So your code has two enumerations, and not every enumerable supports that. – CodesInChaos Apr 11 '11 at 10:39
  • @CodeInChaos: In which case, add a `.ToList()` after the `.Where()`? – Kieren Johnstone Apr 11 '11 at 10:41
0

Single

It returns a single specific element from a collection of elements if element match found. An exception is thrown, if none or more than one match found for that element in the collection.

SingleOrDefault

It returns a single specific element from a collection of elements if element match found. An exception is thrown, if more than one match found for that element in the collection. A default value is returned, if no match is found for that element in the collection.

here is sample example:-

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace LinqSingleorSingleOrDefault
{
class Employee
{
    public int Id { get; set; }
    public string Name { get; set; }
    public string City { get; set; }
}

public class Program
{

    static void Main(string[] args)
    {
        IList<Employee> employeeList = new List<Employee>(){
            new Employee() { Id = 10, Name = "Chris", City = "London" },
            new Employee() { Id=11, Name="Robert", City="London"},
            new Employee() { Id=12, Name="Mahesh", City="India"},
            new Employee() { Id=13, Name="Peter", City="US"},
            new Employee() { Id=14, Name="Chris", City="US"}
        };

        //Single Example

        var result1 = employeeList.Single(); 
        // this will throw an InvalidOperationException exception because more than 1 element in employeeList.

        var result2 = employeeList.Single(e => e.Id == 11);
        //exactly one element exists for Id=11

        var result3 = employeeList.Single(e => e.Name == "Chris");
        // throws an InvalidOperationException exception because of more than 1 element contain for Name=Chris

        IList<int> intList = new List<int> { 2 };
        var result4 = intList.Single(); 
        // return 2 as output because exactly 1 element exists


        //SingleOrDefault Example

        var result5 = employeeList.SingleOrDefault(e => e.Name == "Mohan");
        //return default null because not element found for specific condition.

        var result6 = employeeList.SingleOrDefault(e => e.Name == "Chris");
        // throws an exception that Sequence contains more than one matching element

        var result7 = employeeList.SingleOrDefault(e => e.Id == 12);
        //return only 1 element

        Console.ReadLine();

    }
 }   
}
Nimesh khatri
  • 763
  • 12
  • 29
0

I agree with Kieren Johnstone, don't wait for the exception this is pretty costly, sure when you call this method alot of times.

You're first code snippet is even more expensive, because you wait for the original exception, and than throw yourself a new one.

the_drow
  • 18,571
  • 25
  • 126
  • 193
ChristiaanV
  • 5,401
  • 3
  • 32
  • 42