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;

Is there a way to refactor this?

share|improve this 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: 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
add comment

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.

1 Answer

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;



share|improve this answer
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
add comment

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