LINQ foreach - error handling and general improvement

Question

I'm looking for two kinds of feedback:

1. Is there a better way of doing what I'm attempting to do?
2. Any obvious weaknesses that could potentially cause problems?

The code in question (I've broken it into lines to prevent horizontal scroll):

items.ForEach(i => 
                i.JobOrderTypeString = Context.JobOrderTypes.Where(x => 
                    x.JobOrderTypeIdentifier == i.JobOrderTypeIdentifier.CodeListItemIdentifier)
                    .FirstOrDefault().Title);

Datatypes:

• items is a List of simple ViewModels

My worries:

• If FirstOrDefault() returns null (though very unlikely), and I'm "reading" the .Title property, will my code crash?
• I'm using Context on each iteration, which I'm suspecting is probably not the best idea.

Answer 1

First, the least important: Personally I don't like using the .ForEach extension method as it looks too much like LINQ query commands which are mainly for projection/filtering. I just use a straight-up foreach loop most of the time. But that's just style points.

You can simplify .Where(lambda).FirstOrDefault() to FirstOrDefault(lambda).

You're referencing context every time, but you don't have to, you're right. You could enumerate over context.JobOrderTypes and store it in memory before iterating. Chances are it's going to be more performant than hammering your db every iteration. You are pulling back an entire table into memory, though, so be aware of the implications of that.

Your code is going to crash but you can fix it either very simply:

var val = ...FirstOrDefault(x => x.JobOrderTypeIdentifier == identifier);
i.JobOrderTypeString = val != null ? val.Title : "SomeDefaultValue";

Or you can flatten to an IEnumerable of Titles first (essentially saying "Give me the first of all Titles of objects that match my criteria" instead of saying "Give me the Title of the first object that matches my criteria, if no matches then null"):

i.JobOrderTypeString = context.JobOrderTypes.Where(x => x.JobOrderTypeIdentifier == identifier).SelectMany(j => j.Title).FirstOrDefault();

I'm not an EF expert so I'm not sure if the following will generate the same SQL, but it's cleaner LINQ:

i.JobOrderTypeString = context.JobOrderTypes.SelectMany(j => j.Title).FirstOrDefault(x => x.JobOrderTypeIdentifier == identifier);

Generally code that has a hydrated collection, but then goes through and iterates through the collection and sets values on each element in the collection means that you can do it better. I don't know where this code lives, but make sure it's in a place that says "right before I write these to the database, calculate!" You could also do this computation whenever the column value on i changes or when i is instantiated.

This may be a sign that your problem could be better solved by using a stored proc or a computed column, or you need to wrap the in a "BeforeInsert"-type event.

Answer 2

If FirstOrDefault() returns null (though very unlikely), and I'm "reading" the .Title property, will my code crash?

What else should your code do ? It sure crashes if default(T) == null.

To avoid getting a null reference exception and using the context all the time, you can extract the part where you get the Title to a separate method, where you can cache the results of the earlier queries.

private Dictionary<CodeListItemIdentifier, String> cachedResults = new Dictionary<CodeListItemIdentifier, String>();
private void ClearCache()
{
    cachedResults.Clear(); 
}

private String GetTitle(CodeListItemIdentifier identifier)
{
    String title = null;
    if (cachedResults.TryGetValue(identifier,title)) { return title; }

    JobOrderType jobOrderType = Context.JobOrderTypes
                .Where(x => x.JobOrderTypeIdentifier == identifier)
                .FirstOrDefault();

    if (jobOrderType == default(JobOrderTypeIdentifier)) { return String.Empty; }

    cachedResults[identifier] = jobOrderType.Title;
    return jobOrderType.Title;
}

The problems with this caching is, that changes in the databse won't be reflected. Therefor you need after changes in the database to call the ClearCache() method. You can also call this method each time before you call this linq query.

Speaking about the linq query, here it is

items.ForEach(i => 
                i.JobOrderTypeString = GetTitle(i.JobOrderTypeIdentifier.CodeListItemIdentifier));

Answer 3

The accepted answer is very good. I just wanted to add some more thoughts on this.

Correct me if I'm wrong, but I believe the LINQ is equivalent to this foreach loop?

foreach(var item in items)
{
    item.JobOrderTypeString = Context.JobOrderTypes
        .Where(x => x.JobOrderTypeIdentifier == i.JobOrderTypeIdentifier.CodeListItemIdentifier)
        .FirstOrDefault()
        .Title;
}

For me, this is 10x more readable and makes it a lot easier to refactor.

For example, it's now a bit more obvious that the Where clause is not needed.

foreach(var item in items)
{
    item.JobOrderTypeString = Context.JobOrderTypes
        .FirstOrDefault(x => x.JobOrderTypeIdentifier == i.JobOrderTypeIdentifier.CodeListItemIdentifier)
        .Title;
}

If FirstOrDefault() returns null (though very unlikely), and I'm "reading" the .Title property, will my code crash?

Yes, the null returned from FirstOrDefault could indeed crash the program. You have a couple of options here:

1. If you know with 100% certainty that it will never happen, you should use First instead. The code will still crash if you're wrong, but it's a little more clear that the code was intended to never return null.

   item.JobOrderTypeString = Context.JobOrderTypes
       .First(x => x.JobOrderTypeIdentifier == i.JobOrderTypeIdentifier.CodeListItemIdentifier)
       .Title;
   

2. If it is possible to get null, even when it's unlikely, the code should reflect that. For example:

   var jobOrderType = Context.JobOrderTypes
       .FirstOrDefault(x => x.JobOrderTypeIdentifier == i.JobOrderTypeIdentifier.CodeListItemIdentifier);
   
   if(jobOrderType != null)
       item.JobOrderTypeString = jobOrderType.Title;
   else
       // throw an exception or log a warning or something.