11
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ So ttJobHead is a DataTable ? \$\endgroup\$ – Heslacher Apr 28 '15 at 16:03
  • 1
    \$\begingroup\$ @Heslacher Looks to be that way. \$\endgroup\$ – moarboilerplate Apr 28 '15 at 16:08
  • \$\begingroup\$ @It'sbeenapleasure I made an edit to your post. Let me know if that improves it or roll it back if you don't like it. \$\endgroup\$ – moarboilerplate Apr 28 '15 at 16:10
  • \$\begingroup\$ @Heslacher If you say so... \$\endgroup\$ – moarboilerplate Apr 28 '15 at 16:12
  • 1
    \$\begingroup\$ @200_success you had one job to do! One job! \$\endgroup\$ – MikeTheLiar Apr 28 '15 at 20:25
16
\$\begingroup\$

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.

\$\endgroup\$
  • 5
    \$\begingroup\$ ^^ That. This isn't about efficiency, it's about readability and maintainability. Throwing an exception if there's anything other than a single row in that table conveys the expectations and removes all ambiguity - when the code says "there's one row in that table, or else...", then you don't need a comment to clarify anything. \$\endgroup\$ – Mathieu Guindon Apr 28 '15 at 17:50
  • 2
    \$\begingroup\$ I would say, based on the OP's assertion that there ever will only be 1 row that .Single() is the better match to the intent over .First(). \$\endgroup\$ – Jesse C. Slicer Apr 28 '15 at 17:51
  • \$\begingroup\$ @JesseC.Slicer You're right, the answer should be updated to reflect that. \$\endgroup\$ – moarboilerplate Apr 28 '15 at 17:52
  • \$\begingroup\$ Good points about Single() +1 \$\endgroup\$ – Heslacher Apr 29 '15 at 5:37
  • \$\begingroup\$ I always use .FirstOrDefault and then test for null. Bad? \$\endgroup\$ – Ewan May 1 '15 at 8:41
4
\$\begingroup\$

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

Not really, but you would gain a lot by removing the inner loop.

The inner loop can be removed like

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

The advantage of removing the outer loop will at maximum save you some milliseconds, but won't reduce the amount of code, because you then should add a comment, why you only use the "first" element.

But hey, why could the inner loop be removed ? Very easy, for each iteration of the loop you are overwriting the currentJob.ProjectID property.

\$\endgroup\$
  • \$\begingroup\$ thanks, but in the inner loop there should only ever be a single match so im not really overriding anything in each iteration. \$\endgroup\$ – user28366 Apr 28 '15 at 17:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy