Replacing a foreach with LINQ expression

Question

So I have a code like this:

foreach (var optionValues in productOption.ProductOptionValues)
{
  if (optionValues.ProductOptionValueID > 0)
  {
    unitOfWork.ProductContext.Entry(optionValues).State = EntityState.Modified;
  }
  else
  {
    unitOfWork.ProductContext.Entry(optionValues).State = EntityState.Added;
  }
}

The code review for this was that I should look at using LINQ to do this.

Can someone please point me to a resource that can explain using LINQ to change the object properties?

I'd like see the justification for using LINQ here. LINQ is a *query* syntax so in your circumstance doesn't really fit. However, with that being said here is a simple [ForEach](http://stackoverflow.com/questions/200574/linq-equivalent-of-foreach-for-ienumerablet) extension you could use to do it. — James, Oct 07 '14 at 10:05
What you may want to use [List.ForEach](http://msdn.microsoft.com/en-us/library/bwabdf9z(v=vs.110).aspx) but still your original code is good. — Renatas M., Oct 07 '14 at 10:07
Is `productOption.ProductOptionValues` a `DbSet` in EntityFrameWork? — Masoud, Oct 07 '14 at 10:08
The only change I might make is to fold the if/else condition into a ?: statement, so you don't have a repetition of the whole property path: `unitOfWork.ProductContext.Entry(optionValues).State = option.ProductOptionValueID > 0 ? EntityState.Modified : EntityState.Added;` — Avner Shahar-Kashtan, Oct 07 '14 at 10:09

score 8 · Answer 1 · answered Oct 07 '14 at 10:05

8

You don't. Simple as that.

The code review for this was that I should look at using LINQ to do this and avoid the foreach.

Tell the code reviewer he is wrong. LinQ is for Querying data. You are updating data. Stay with your foreach loop, it's fine.

answered Oct 07 '14 at 10:05

nvoigt

75,013
26
93
142

score 7 · Answer 2 · answered Oct 07 '14 at 10:04

7

LINQ is for querying. You are modifying values, so foreach is perfectly fine here.

answered Oct 07 '14 at 10:04

Heinzi

167,459
57
363
519

Enigmativity · Answer 3 · 2014-10-07T10:39:45.960

The best you could do is this:

var query =
    from optionValues in productOption.ProductOptionValues
    select new
    {
        entry = unitOfWork.ProductContext.Entry(optionValues),
        value = optionValues.ProductOptionValueID > 0
            ? EntityState.Modified
            : EntityState.Added
    };

foreach (var x in query)
{
    x.entry.State = x.value;
}

But I don't think that this really gives you much in terms of readability.

score 0 · Answer 4 · answered Oct 07 '14 at 10:10

The only reasonable use of LINQ here (and it depends on the type of ProductOptionValues) is to filter the results using Where, which essentially replaces your if statement, but it's not better than your current code:

foreach (var option in productOption.ProductOptionValues.Where(x => x.ProductOptionValueID > 0)
    unitOfWork.ProductContext.Entry(optionValues).State = EntityState.Modified;

foreach (var option in productOption.ProductOptionValues.Where(x => x.ProductOptionValueID <= 0)
    unitOfWork.ProductContext.Entry(optionValues).State = EntityState.Added;

score 0 · Answer 5 · answered Oct 09 '14 at 12:24

You should not use the LINQ ForEach extension. Let me explain why:

The LINQ foreach violates the functional programming principles that all the other sequence operators are based upon. Clearly the sole purpose of a call to this method is to cause side effects. The purpose of an expression is to compute a value, not to cause a side effect. The purpose of a statement is to cause a side effect. The call site of this thing would look an awful lot like an expression

The second reason is that using it adds zero representational value to your code. Doing this lets you rewrite this perfectly clear code:

foreach(Foo foo in foos){ statement involving foo; }

into this code:

foos.ForEach((Foo foo)=>{ statement involving foo; });

which uses almost exactly the same characters in slightly different order. And yet the second version is harder to understand, harder to debug, and introduces closure semantics, thereby potentially changing object lifetimes in subtle ways.

The above is in parts a summary of a blog Post from Eric Lippert. Read the full post here.

What is more the Extension has been removed by the BCL Team in Windows 8:

List.ForEach has been removed in Metro style apps. While the method seems simple it has a number of potential problems when the list gets mutated by the method passed to ForEach.

Instead it is recommended that you simply use a foreach loop.

score -1 · Answer 6 · answered Oct 07 '14 at 10:13

-1

Assuming productOption.ProductOptionValues is an IList<>() (if it isn't you might need to do a .ToList() before the .ForEach), it would be something like this:

productOption.ProductOptionValues.ForEach(x => 
   unitOfWork.ProductContext.Entry(x).State = (
      (x.ProductOptionValueID > 0) ? EntityState.Modified : EntityState.Added)
)

... but I don't think that's really an improvement. Quite the opposite, in fact.

Really, don't do this.

answered Oct 07 '14 at 10:13

BCdotWEB

1,009
1
14
35

2

Strictly speaking, this is not even LinQ, as `ForEach` is a method of `List` and not an extension method from `System.Linq`. – nvoigt Oct 07 '14 at 10:26

score -1 · Answer 7 · answered Oct 07 '14 at 10:16

Tricky, just for the humor, it is possible in several ways, such as:

var sum = productOption.ProductOptionValues.Select(
         optionValues => unitOfWork.ProductContext.Entry(optionValues).State = (optionValues.ProductOptionValueID > 0 ? EntityState.Modified : EntityState.Added).Sum();

Replacing a foreach with LINQ expression

7 Answers7