3

When you know id is suppose to exist as in the example below, is it good practice to check for nulls? 

var submission = _ctx.Submissions.FirstOrDefault(p => p.Id == id); submission.Done = true; _ctx.Submissions.Attach(submission); _ctx.Entry(submission).State = EntityState.Modified; _ctx.SaveChanges();
Improve this question
6
  • 4
    Almost always when you use FirstOrDefault() you should check for null immediately. Commented Aug 28, 2014 at 16:41
  • Is there a reason not to check for null? If submission was null, could you still do something useful? Commented Aug 28, 2014 at 16:41
  • Attach and changing State are redundant, the object returned from DbSet<T> is tracked by change tracker Commented Aug 28, 2014 at 16:45
  • Any time you are dealing with a reference type and it's unacceptable to throw NullReferenceException, you should check for null. Commented Aug 28, 2014 at 16:49
  • @PrestonGuillot - If system is in unexpected state it is perfectly fine to let any any type of exception to be thrown. As OP said - this particular code never expected to return null - so no need to check ok to let whole request to fail. (I don't think FirstOeDefault is a good choice of method as I put in my answer). Commented Aug 28, 2014 at 16:53

6 Answers 6

Reset to default
4

If id is supposed to exist you do not check for null. In fact in this case you should query like this

_ctx.Submissions.Single(p => p.Id == id);

The Single method will throw an exception if the item is not found. On the other hand if you are building a WebAPI or have a specific not found page as the id is sent by the client (i.e. can result by something outside your code) you may use SingleOrDefault, then check for null and return appropriate HTTP status code or redirect to the appropriate page. Basically if null results from not validated data you should check and respond accordingly. If the arguments that cause null values are generated by code you should NOT check for null and in fact query in a way that may not result in null because otherwise you are just covering an error in your code.

This is my opinion which is a result of my personal experience. Note that not everyone agrees as is obvious by the different answers of a question I posted on programmers.stackexchange.com some time ago - https://softwareengineering.stackexchange.com/questions/147480/should-one-check-for-null-if-he-does-not-expect-null

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

3

You check for null when you cannot say with complete certainty that the object will never not be null.

With that said, you are using FirstOrDefault(). If it didn't find anything in the criteria, it will return you null.

Improve this answer

4 Comments

The only way the id will not exist will be because of someone has gone in the DB and physically removed it (orphan record so to speak). As a developer, do I need to cater for 'mistake-updates' in the database?
@DrZeuso You need to cater for stuff you know could happen and might cause the program to fail ungracefully. If you know id could be null, then you should handle it so. You don't want stuff to crash the program. This is falling slightly into "opinion based" territory as some will want to handle with custom exceptions in try/catch or check for null and add in additional business logic code.
How about in case of, data will be good in Production env, but because we have crap data currently in DEV (null fields that shouldn't be etc), it's making my code crash in DEV. Do I fix up the data in DEV so it's not null so my program runs correctly? or check for nulls in my program?
@DrZeuso I see no reason why one should not handle errors that they know about. If you're fine with a top level try/catch catching them then there's nothing to change as long as it handles the error the way you want it to.
2

If you feel that there should always be an object, and that there not being an object in question would mean that there is a bug in the program somewhere, then you should use First not FirstOrDefault. Doing this means that if one of your assumptions for this code ends up being violated you will be informed quickly, loudly, and as close to the source of the problem as possible.

FirstOrDefault should be used if it's entirely sensible for the query in question to have no items, resulting in a null value being returned. If you are in this position, the program should work correctly if the query does in fact return null, or whatever else you need to do for the program to function appropriately.

What you want to avoid is putting yourself in the position where a program (incorrectly) assumes a query always has at least one item, but where you use FirstOrDeafult anyway. Here you end up with NullReferenceExceptions, which can take extra debugging work to figure out where the actual problem is (a query with no items that should have had items).

Improve this answer

Comments

1

For resilient code you should definitely check and handle to see if submission is null.

Improve this answer

Comments

0

You check for the null values, if there is an error to be occured by passing a null value. You can check them in any way, either by using try catch block or using a an if else block.

You can check it, this way you can prevent the errors that might occur by a null value.

Improve this answer

Comments

0

FirstOrDefault documents your intention as "the result I'm expecting may not contain items". In such case you should check for null and perform some reasonable fallback. Throwing exception at that point would look strange.

It sounds like in your case you don't expect empty result, so use First and handle exception somewhere higher on call stack or even let default exception handling to happen.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.