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,ToListnever returnsnull. It'll return an empty collection or will throw anArgumentNullException if the enumerable isnull.

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

I feel like you should split both yourif in different methods, maybeGetUserCustomBehavior andGetOrganizationCustomBehavior. Then, your code woudl be cleaner.

// GET: api/CustomBehaviourspublic 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 thevar keyword.var is cool, but tools like Resharper indicate they should be usedonly when you cansee which type is used ex :

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

Also, you don't really need to check forCount > 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 lessif, 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.

• 1
  \$\begingroup\$Awesome answer and extremely informative. Thank you.\$\endgroup\$

  Carlos P

  – Carlos P

  2015-09-30 17:17:08 +00:00

  CommentedSep 30, 2015 at 17:17

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.

• c#
• .net
• linq