I regularly review the code that I maintain. Recently, I’ve come across code like this fairly often:
someCollection.FirstOrDefault().Id
I cannot rightly comprehend why anyone would do this.
FirstOrDefault() returns the first item in a sequence or the default value if it doesn’t exist (i.e. the sequence is empty). For a reference type (classes, basically) the default value is null. So using the value returned by FirstOrDefault() without a null check is only valid for when the sequence contains a value type (e.g. int, decimal, DateTime, Guid, etc.)
In the example above if someCollection is an empty list/array/collection/whatever then FirstOrDefault() will return null and the call to the Id property will fail.
Then you are left with a NullReferenceException on line xxx but you don’t know if it is someCollection, or the returned value from FirstOrDefault() which then wastes your time (or the time of someone else who is having to debug it).
So, if the sequence must always contain items then use First(), in the exceptional event that it is empty the call to First() will throw a more appropriate exception that will help you debug faster. If it is perfectly valid for the sequence to be empty then perform a null check and change the behaviour appropriately.
The Blog of Colin Mackay 
This method answers the request: if you have anything there, give me the first one.
It’s a legitimate API.
The problem is that is widely used in replacement of Single() which serves entirely different purposes.
The problem with isn’t that value or default is being requested here, that is the correct behaviour.
The problem is that, firstly, it’s generally thought to be good OO practice *not* to return nulls so that null checking becomes unnecessary. In this case -as far as I see it- this method is stating in it’s signature that it has no null return and it is safe for me to assume that if no objects are available, it will provide an expected default object so I may continue without having to check it’s result every time it’s used. For example, if you have a method which returns a list of transactions for your top sales person today ‘getFirstOrDefault()’ would return the top sales persons sales today or an empty list if there were no sales. Returning null here would be silly. So if this isn’t what the method you mention is doing the it is simply misnamed. Otherwise there is no problem (with the method idea in question at least).
Secondly, accessing members of returned objects is horrible. It makes locating null pointers a pain and reading the code a pain.
Going by your comments, I’m assuming you are not familiar with C# or the LINQ framework as the nomenclature does fit with the C# language specification.
“this method is stating in it’s signature that it has no null return and it is safe for me to assume that if no objects are available, it will provide an expected default object” – Actually, what is is returning is the result of
default(T), where T is a generic type which is inferred by the type of the sequence. For reference types this isnull. Anintis0, aboolisfalse, and so on.If you want the behaviour you are talking about you need to use
DefaultIfEmpty()[docs here] and supply the default object you want back in the case of an empty sequence.I’d really rather null was abolished from C# except from explicit usage of ? after a type declaration.
Some use cases may call for this, but with a default value different from null: have a look at this stackoverflow question I asked some while ago, answers to which helped me tremendously afterwards. I’ll take a “DefaultIfEmpty” in my linq chain over “if (result == null)” anyday 🙂
I forgot the link, or it was eaten by the comment system: http://stackoverflow.com/questions/11222757/safely-dereferencing-firstordefault-call-in-linq-c-sharp
Yes, that’s a really good way of handling cases where you want to substitute a more specialised default if the situation calls for it.
I solved this problem by extending the Any method to check for nulls like so:
public static bool AnyNotNull(this IEnumerable source)
{
if (source == null)
return false;
return source.Any();
}
public static bool AnyNotNull(this IEnumerable source, Func predicate)
{
if (source == null)
return false;
return source.Any(predicate);
}
Then use:
var id = someCollection.AnyNotNull() ? someCollection.First().Id : -1;
Or:
var myId = 1000;
var myObject = someCollection.AnyNotNull(o => o.Id == myId) ? someCollection.First(o => o.Id == myId) : defaultObject;
Yep I’ve always had AnySafe() methods to go alongside my TrimSafe() method (which would use AnySafe())
Excluding UnitTests it is almost always wrong to use FirstOrDefault. It is the stick your head in the sand method. SingleOrDefault can be an acceptable usage.
The LINQ methods of First / Single are extremely unhelpful for their meaningless error messages they create. When i find myself needing First or Single i always create extension methods that allow me to do dollarDollarBills.Single(x=> x.TrillionDollarBill, “trillion dollar bill”) that will throw an exception “expected to have exactly 1 ‘trillion dollar bill’ however found #”.
Using First or Single without custom exception results… you will hate life later.
“Excluding UnitTests it is almost always wrong to use FirstOrDefault. It is the stick your head in the sand method. SingleOrDefault can be an acceptable usage.”
Is that so? Given a Person class with a Children property that is the collection of children of that person ordered by birth date, how do you answer the question “who’s the eldest son?”?
That’s exceedingly arbitrary and I doubt the value of that exists anywhere other than a homework problem. But playing along if(children.Any()) return children.First(). This is actually a great illustration of my point. You are likely in a vastly different scenario for a person who has children vs a person who has none. There is likely no value in carrying around var eldestChild = (child)null. I wouldn’t be in the code path at all for eldestChild if a person had no children.
Are you aware that LINQ works also over queriables (IQueriable) which, usually, are executed remotely? Your solution would require to go to the server 2 times instead of 1. And if it’s a billable hosted service, it would not only take twice as long, it would cost twice as much.
I think you’re really stretching now that you’re moving out of LINQ-to-objects and going to LINQ-to-whatever. I digress, your statement does not apply to how i build software. I am extremely greedy when it comes to data. I am quite content to bring back all the data i could possibly need in 1 request and throw away all i do not need. In general it is always 1 request in = at most 1 read request to db and at most 1 write request to the db. There might be rare occurrences that need me to make 2-3 requests but those are far and few in-between. The primary reason is I do not use relational systems for OLTP. If i found myself needing 4+ write transactions it would be in an eventing model over a message bus and decoupled from any in bound request timeline. My read model is always based on facilitating the best UX in exactly 1 read, when possible not even leaving memory of the webserver. Everything is eventually consistent.
So much truth.
The other thing in other people’s code that is similarly mystifying: (obj as MyEntity).Id.
A NullReferenceException deep in library code is the least helpful way to diagnose a problem. If you need obj to be MyEntity in order for your code to succeed, I need to know that via an InvalidCastException! And like you said, if you need the list I passed you to be non-empty, NullReferenceException won’t tell me what is wrong either!
If you need to do (obj as MyEntity).Id there’s much bigger problems than fear of a NRE.