patterncsharpModerate
I had one job to do
Viewed 0 times
onehadjob
Problem
The code I am working with:
equivalent to the following pseudo code:
I know for sure that the
My question is is there any advantage storing reference to the
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.
Note that
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.
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.