HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

I had one job to do

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
onehadjob

Problem

The code I am working with:

foreach(var currentJob in ttJobHead)
{
    foreach (var hdCase in Db.HDCase.Where(hdCase => hdCase.HDCaseNum == currentJob.HDCaseNum))
    {
        currentJob.ProjectID = hdCase.ProjectID;
    }
}


equivalent to the following pseudo code:

foreach(var t1Row in Table1)
{
    foreach (var t2Row in Table2.Where(r => r.Property1 == t1Row.Property1))
    {
        t1Row.Property2 = t2Row.Poperty2;
    }
}


I know for sure that the Table1 will only ever have 1 row, therefore the outer foreach will only ever iterate once.

My question is is there any advantage storing reference to the t1Row in a variable rather having a foreach loop?

Solution

I would say yes, there is value in removing the foreach loop, because your code will more appropriately mirror what you're expecting to happen versus what can happen. It's unclear to someone looking at this code for the first time that you're really doing a lookup based on one value as opposed to a join.

I would go a step further than storing the reference to table 1. I would actually store the row and then query table 2 for matches, e.g.

var currentJob = ttJobHead.Single();
foreach(var hdCase in Db.HDCase.Where(hCase => hCase.HDCaseNum == currentJob.HDCaseNum))
{
    hdCase.ProjectID = currentJob.ProjectID;
}


Note that Single() aggressively throws an exception when there's not a matching row. That's a good thing in a lot of cases. It fails quickly as soon as your data model changes and also clearly states to developers "an empty table makes an exception throw. Don't support it unless you intend to change that requirement." Of course you can change what you support by changing the code to use SingleOrDefault(), First(), or FirstOrDefault().

You could also merge the assignment statement into the foreach statement but it could make tracking exceptions more difficult.

I also noticed you mentioned in a comment that you intend to only find one matching record. If that is the case then remove the second foreach loop, add Single() to the end of the LINQ expression, and deal only with that record. That way you make it abundantly clear there is a 1:1 relationship between rows in these tables, no more, no less.

Code Snippets

var currentJob = ttJobHead.Single();
foreach(var hdCase in Db.HDCase.Where(hCase => hCase.HDCaseNum == currentJob.HDCaseNum))
{
    hdCase.ProjectID = currentJob.ProjectID;
}

Context

StackExchange Code Review Q#88237, answer score: 16

Revisions (0)

No revisions yet.