3

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

  • @JaCquesB, What happens if there is nothing returned? – w0051977 Jul 8 '17 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 '17 at 12:42
  • Once you let nulls in you're forever needing to test for null. So have you faithfully prevented nulls from coming in else where? – candied_orange Jul 8 '17 at 12:45
  • @CandiedOrange. I have to check the database to find out if an application exists. – w0051977 Jul 8 '17 at 12:57
  • 2
    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. – Ben Cottrell Jul 8 '17 at 13:04
8

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

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.
  • 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. – Derek Elkins Jul 8 '17 at 22:45
0

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!

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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