Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my code that works:

foreach (var r in rlist)
    {
        if (r.IndexOf("_") != -1)
        {
            int id = int.Parse(r.Split('_')[1]);
            var x_tmp = (from x in db.tblX where x.x_id == my_id && x.x_id == id select x).First();
            x_tmp.order = someNumber;
        }
     }
     db.SaveChanges();

Is there a way to refactor this?

share|improve this question

closed as off-topic by Jamal Dec 15 '13 at 3:15

  • This question does not appear to be a code review request within the scope defined in the help center.
If this question can be reworded to fit the rules in the help center, please edit the question.

    
Is it supposed to set the same order for all the items? Shouldn't you be incrementing the order? –  ANeves Nov 19 '12 at 8:59
1  
-1: also, why are you comparing x.x_id to both my_id and id? Does this code compile? Does it work as intended? Did you try to improve it by yourself before putting it on our collective table? –  ANeves Nov 19 '12 at 9:03
    
This question appears to be off-topic because it only asks for new refactored code instead of ideas for refactoring the original code. –  Jamal Dec 15 '13 at 3:15

1 Answer 1

up vote 4 down vote accepted

Seems you are calling the query a few times with id, so let's just call it only once with all of them.

(from x in db.tblX where x.x_id == my_id && ids.Contains(x.x_id) select x)

There you go, now IEnumerable's Contains function allows you to look up the id in a list of ids you have made in advance. I usually prefer to use LINQ methods, as simple LINQ expressions tempt to be more complex than they should be:

db.tblX.Where(x => x.x_id == my_id && ids.Contains(x.x_id))

We can now use a plain foreach as suggested in comments, such that we don't have side effects:

var ids = rlist.Where(x => x.IndexOf("_") != -1).Select(x => int.Parse(x.Split('_')[1]));

var items = db.tblX.Where(x => x.x_id == my_id && ids.Contains(x.x_id));

foreach (var item in items)
    item.order = someNumber;

db.SaveChanges();

Done.

share|improve this answer
2  
Why are you suggesting ForEach()? I think foreach is more idiomatic and readable. And ForEach() also clashes with the idea of side-effects-free lambdas, that's behind LINQ. Also, your first ForEach() doesn't work, you want to use Select() there. –  svick Nov 18 '12 at 18:33
    
Adapted answer to your feedback, thanks! Seems I need to think some more about avoiding side effects... –  Tom Wijsman Nov 18 '12 at 18:53

Not the answer you're looking for? Browse other questions tagged or ask your own question.