LINQ filtering query

Question

I often find myself writing LINQ queries like this. I end up testing for Null and zero-counts pretty often, and the code sprawls out to be something like the following:

// GET: api/CustomBehaviours
public IEnumerable<CustomBehaviour> Get(int? orgID = null, long? userID = null)
{
 using (PAMDatabase db = new PAMDatabase(WebApiConfig.PAMConnectionString))
 {
 List<CustomBehaviour> output = new List<CustomBehaviour>();
 // Add all custom behaviours for the specified user
 if (userID != null) {
 var usr = db.context.users.AsNoTracking().Where(u => u.userID == userID).FirstOrDefault();
 if (usr != null)
 {
 var cbs = usr.customBehaviours.ToList();
 if ((cbs != null) && (cbs.Count > 0))
 output.AddRange(Mapper.Map<List<CustomBehaviour>>(cbs));
 }
 }
 // Add all custom behaviours for the specified organisation
 if (orgID != null)
 {
 var org = db.context.orgs.AsNoTracking().Where(o => o.orgID == orgID).FirstOrDefault();
 if (org != null)
 {
 var cbs = org.customBehaviours.ToList();
 if ((cbs != null) && (cbs.Count > 0))
 output.AddRange(Mapper.Map<List<CustomBehaviour>>(cbs));
 }
 }
 return output;
 }
}

Can this be made more concise? I'm tempted to remove all the Null checks and wrap the whole thing in a big try { } catch {} but I believe that goes against the conventional use of exceptions.

• ANeves: .ToList() must not return nulls; it should return a list, which can be empty or with items.
• Carlos P: @Heslacher - No, cbs is a List<customBehaviour>, which is an entity object. CustomBehaviour is the corresponding Data Transfer Object.
• Thomas Eyde: I would skip the if ((cbs != null) && (cbs.Count > 0)) all together. .ToList() will never return null, and adding an empty list to output is something you don't need to optimize. Compared to the time it takes to query the database, it's nothing.

Answer 1

FirstOfDefault can also take a filtering query, just so you know.

Which means :

db.context.users.AsNoTracking().Where(u => u.userID == userID).FirstOrDefault();

can be :

db.context.users.AsNoTracking().FirstOrDefault(u => u.userID == userID);

If you use C#6, you can use the null propagation operator ?. instead of multiple null checks :

if (usr != null)
{
 var cbs = usr.customBehaviours.ToList();
 if ((cbs != null) && (cbs.Count > 0))
 //...

is now :

if (usr?.customBehaviours != null && cbs.Count > 0)

As pointed @ANeves in the comments, ToList never returns null. It'll return an empty collection or will throw an ArgumentNullException if the enumerable is null.

So, doing .ToList() before checking if usr.customBehavious == null won't save you.

I feel like you should split both your if in different methods, maybe GetUserCustomBehavior and GetOrganizationCustomBehavior. Then, your code woudl be cleaner.

// GET: api/CustomBehaviours
public IEnumerable<CustomBehaviour> Get(int? orgID = null, long? userID = null)
{
 using (PAMDatabase db = new PAMDatabase(WebApiConfig.PAMConnectionString))
 {
 List<CustomBehaviour> output = new List<CustomBehaviour>();
 // Add all custom behaviours for the specified user
 if (userID != null) {
 var userBehaviors = GetUserCustomBehaviors(userID.Value, db);
 output.AddRange(Mapper.Map<List<CustomBehaviour>>(userBehaviors));
 }
 // Add all custom behaviours for the specified organisation
 if (orgID != null)
 {
 var orgBehaviors = GetOrganizationCustomBehavior(orgID.Value, db);
 output.AddRange(Mapper.Map<List<CustomBehaviour>>(orgBehaviors));
 }
 return output;
 }
}
private IEnumerable<???> GetUserCustomBehavior(int? userId, PAMDatabase db)
{
 var usr = db.context.users.AsNoTracking().FirstOrDefault(u => u.userID == userID);
 return usr?.customBehaviours ?? new List<???>();
}
private IEnumerable<???> getOrganizationCustomBehavior(int orgId, PAMDatabase db)
{
 var org = db.context.orgs.AsNoTracking().FirstOrDefault(o => o.orgID == orgID);
 return org?.customBehavior ?? new List<???>();
}

Now, notice something, I wasn't able to see which type was used in your code because of the var keyword. var is cool, but tools like Resharper indicate they should be used only when you can see which type is used ex :

//good
var list = new List<int>();
//not good
var user = FooTheBar();

Also, you don't really need to check for Count > 0, what's the worst that can happen, you'll add an empty range to your list? That's usually not important, and your code has less if, which is good.

I'm no pro with Automapper, but I don't think it's a good practice to map lists, maybe you should define your map per object (Mapper.CreateMap<???,CustomBehaviour>()) but maybe there's a performance gain I'm not aware of.

• Carlos P: Awesome answer and extremely informative. Thank you.

Answer 2

Maybe a little bit off topic but maybe it will help. I see you are using FirstOrDefault where, if I'm correct, SingleOrDefault would be more semantic.

db.context.users.AsNoTracking().Where(u => u.userID == userID).FirstOrDefault(); 

As you are querying for a userID, which I assume, is unique in the users table, SingleOrDefault states that there can be only one user what the queried userID.

When using FirstOrDefault you basically saying that there could be more users with the same userID, and if so then just randomly (because you are not ordering) return one.

About Automapper, I agree with TopinFrassi that mapping a list feels it be odd because it's not re-usable. If you create a mapping between your entity and the CustomBehaviour class, you could do something like:

output.AddRange(cbs.Select(x => Mapper.Map<CustomBehaviour>(x)));

this way you can re-use the mapping definition.

Just my opinion, but I'm not a fan of Automapper's kind of magic. When using these tools you won't get any help from the compiler when renaming or change types of properties. e.g. If you rename a property in your Entity but you forget to rename the same property in your CustomBehaviour class. you won't notice until run time that the mapping has failed.

If you would you a manually made mapping, which tends to also be faster, you still won't have your property name changed, but you will still get your data.

The same thing happens when change a property type, with Automapper you won't notice until run time, manual mapping will give you a compile time error.

Of course the benefit for using Automapper are there also.