2

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

9
  • @JaCquesB, What happens if there is nothing returned?
    – w0051977
    Jul 8, 2017 at 12:29
  • Sorry, I was wrong, .DefaultIfEmpty().First() will yield null if the list is empty, but using .FirstOrDefault() would be simpler. What do you mean by "empty object", do you mean an empty list?
    – JacquesB
    Jul 8, 2017 at 12:42
  • 1
    Once you let nulls in you're forever needing to test for null. So have you faithfully prevented nulls from coming in else where? Jul 8, 2017 at 12:45
  • @CandiedOrange. I have to check the database to find out if an application exists.
    – w0051977
    Jul 8, 2017 at 12:57
  • 3
    Personally I don't think 'empty' objects make a whole lot of sense with regard to databases because the existence of an object in a database is a binary condition - i.e. either it exists or it doesn't exist. That condition is neatly represented in a null-vs-not-null check. Furthermore, the 'Safe Navigation' operator in C# 6.0 can tidy some of the if-not-null clutter. Jul 8, 2017 at 13:04

4 Answers 4

10

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()).

2
  • 2
    This 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
    Jul 9, 2017 at 20:14
  • @RubberDuck: That can be solved by a simple .ToList() after the where, before the any.
    – Flater
    May 17, 2019 at 12:59
3

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-debug NullReferenceExceptions 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 an out parameter. Advantages: This pattern is already known from say Dictionary.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 builtin Option<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.
1
  • 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 the Option approach. Since nulls are, unfortunately, already ubiquitous in C#, I may use null in a temporary, localized way. Jul 8, 2017 at 22:45
1

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.
4
  • Thanks. Could you clarify what you mean by: "move your mapping to after the select!"
    – w0051977
    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 thing
    – Ewan
    Jul 8, 2017 at 13:13
  • although it may be optimised underneath
    – Ewan
    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
    Jul 8, 2017 at 13:26
0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.