Say I have a LINQ query like this:
application = CreditDatabase
.Applications
.Select(Mapper.Map<Application>)
.Where(c => c.uID == urID)
.DefaultIfEmpty().First();
It returns null if the LINQ query returns an empty result set. Is this "correct". Is it better to return an empty object? If so then how can I do that? This link says it is not possible to return a non-null reference type: https://msdn.microsoft.com/en-us/library/bb360179(v=vs.110).aspx
4 Answers 4
Database queries return result sets. An empty set is a reasonable answer; it means you don't have any of the things searched for.
I would argue that your code is going to far in canonicalization of the result.
Your code (somewhere shortly after this line) is going to check to see if the application you just searched for exists or not, so you are not preventing the subsequent conditional logic (not shown in your question) in your code by doing .FirstOrDefault()
.
You can — and I argue should — instead: check for the empty set (i.e. use the size of the result set, maybe with .Any()
) to see if the application exists. That is substantially cleaner than converting the empty set to default and then taking the first and finally later checking that for null to see if the application of interest exists.
Rather than going this far:
application = CreditDatabase
.Applications
.Select(Mapper.Map<Application>)
.Where(c => c.uID == urID)
.DefaultIfEmpty().First();
if ( application == null ) {
// handle missing application, maybe add it...
} else {
// use the found application
}
Omit the default handling; instead:
search = CreditDatabase
.Applications
.Select(Mapper.Map<Application>)
.Where(c => c.uID == urID);
if ( ! search.Any () ) {
// handle missing application, maybe add it...
} else {
application = search.Single ();
// use the found application
}
Addendum: In the above application = search.Single ()
, I have used .Single()
instead of .First()
since excepting if the non-empty result set has more than one match makes sense here: it is a lookup by what we presume is an exact match of unique primary key (otherwise as @JacquesB points out, since the query isn't ordered, we don't really know what we're keeping and what we're throwing away with .First()
).
-
2This is a nice solution. Do you know if that's going to issue two separate queries to the database though? (I'm betting it will.) Hitting the database twice may be undesirable.RubberDuck– RubberDuck2017年07月09日 20:14:39 +00:00Commented Jul 9, 2017 at 20:14
-
@RubberDuck: That can be solved by a simple
.ToList()
after the where, before the any.Flater– Flater2019年05月17日 12:59:40 +00:00Commented May 17, 2019 at 12:59 -
Using Any and then using First/Single is not a suggested pattern since it creates unnecessery queries compared to using FirstOrDefault and then checking for null/default. Using a ToList to first bring all the results locally as a workaround would be an overkill unless one known there is only 1 (or very few) or 0 items returnedGeorge Birbilis– George Birbilis2024年06月20日 23:37:19 +00:00Commented Jun 20, 2024 at 23:37
It depends!
But first a clarification: .DefaultIfEmpty().First()
can be written simpler as .FirstOrDefault()
which does the same - returns the first item, or null
if the result is empty. But you probably have a bug here: First()
indicates you might have multiple items and want to select the first item - but since there is no ordering involved it means you will get a random item. This is probably not what you want! If you really expect there could be multiple matches, then just return the list of matches. If you expect a single item, use Single()
instead, and if you expect either a single item or nothing, use SingleOrDefult()
which will also return null
if there was no match.
Now to the interesting question: What to return from a method which finds either a single object or nothing?
There are different options depending on what you want to achieve:
Return
null
if the item is not found. Advantages: this is really simple. Disadvantages: The client have to check for null which is easily forgotten becaus the type system does not enforce this check. This might lead to hard-to-debugNullReferenceExceptions
down the line. Not recommended.Throw an exception if the object is not found. Advantages: Simple and safe. Disadvantages: Should only be used if it indicates a bug if the object does not exist.
Create a separate method which allow you to check if the object exists (eg.
if (ApplicationExists(id))...
). This method returns a boolean. Advantages: Clear code on the client side. Disadvantages: Will hit the database twice unless you use some kind of caching, so it may be expensive. Also more code.Use the
TryXXX
pattern. Returns a boolean which indicates if the object exists and places the object (if present) in anout
parameter. Advantages: This pattern is already known from sayDictionary.TryGetValue()
so readers will not be confused. Disadvantage: The out parameter leads to somewhat ugly code, although this is improved in C# 6.Return an
Option<T>
type. This requires the client to explicitly check and unwrap the object. Advantages: Much safer and more elegant than using null. Disadvantage: C# does not have a builtinOption<T>
type, you have to find one or write one yourself.
The only thing you should not do:
- return an "empty" instance of the class.
-
An "empty" instance does make sense in many cases e.g. loading a draft state or many situations where things are aggregatable, however, a name like
application
strongly suggests this isn't the case for this example. Boolean blindness argues against approaches via booleans. As you state, I would throw an exception if not getting a result should "never happen", otherwise I would use theOption
approach. Since nulls are, unfortunately, already ubiquitous in C#, I may usenull
in a temporary, localized way.Derek Elkins left SE– Derek Elkins left SE2017年07月08日 22:45:18 +00:00Commented Jul 8, 2017 at 22:45
I like retuning null when something is not found. To me it makes sense.
Give me the first item matching this criteria.. there are none, here is a null. Your code will throw an exception if it tries to use it.
An alternative if you don't like testing for nulls, or empty objects, though is to always return a list of objects.
That way you can do a for each on the list and it naturally does nothing if nothing is found.
There is no way to override the default behaviour, but you can use null concatenation
var x = Applications.FirstOrDefault() ?? new Application();
Ps. move your mapping to after the where clause!
ie
application = CreditDatabase
.Applications //big long list
.Where(c => c.uID == urID) //just the matching one
.Select(Mapper.Map<Application>) //only run the mapper once
.FirstOrDefault() ?? new Application() //return a new application if there was no matching one.
-
Thanks. Could you clarify what you mean by: "move your mapping to after the select!"w0051977– w00519772017年07月08日 13:11:40 +00:00Commented Jul 8, 2017 at 13:11
-
edited to make clearer. it looks like you run the mapper against all the objects and then just select one. better to select the one first and only map the one thingEwan– Ewan2017年07月08日 13:13:45 +00:00Commented Jul 8, 2017 at 13:13
-
although it may be optimised underneathEwan– Ewan2017年07月08日 13:14:18 +00:00Commented Jul 8, 2017 at 13:14
-
Could you clarify what you mean by: "move your mapping to after the select!". Some code would help. Thanks.w0051977– w00519772017年07月08日 13:26:09 +00:00Commented Jul 8, 2017 at 13:26
There are two different situations:
"Give me the item such that X" - should return one item, or nil if the item does not exist. Only for queries that can never have two or more results.
"Give me all items such that X" - should usually return a set, or an array, or a dictionary of items; having two or more items is normal, having zero items is normal as well and therefore returns an empty list.
You can have mixed cases: "Give me a list of all purchases over 100ドル by customer with id abcd". In this case, you can return nil if the customer doesn’t exist, and some list, empty or not, if the customer exists. Assuming that you agree with the caller that a customer not existing, and a customer existing but not having any expensive purchases, are different situations.
.DefaultIfEmpty().First()
will yieldnull
if the list is empty, but using.FirstOrDefault()
would be simpler. What do you mean by "empty object", do you mean an empty list?