current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote -3 down vote favorite

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
    
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

1 Answer 1

up vote 5 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 tend 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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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