LINQ? Refactoring foreach...

Question

Is there a better way to write this? I feel like I'm getting rusty with C# after doing a lot of JavaScript lately. Can this be improved?

    foreach (var item in this.CartItems)
    {
        if (item.EffectivePrice != null)
        {
            this.CartItems[this.CartItems.IndexOf(item)].EffectivePrice = 
                CurrencyHelper.GetLocalizedCurrency(item.EffectivePrice);
        }
    }

Answer 1

I think you can do something like this:

foreach (var item in this.CartItems.Where(i => i.EffectivePrice != null)) 
{ 
        item.EffectivePrice =  
            CurrencyHelper.GetLocalizedCurrency(item.EffectivePrice); 
}

Answer 2

Well, you could write it in LINQ query syntax with a from and a where, but I'm not sure it is a big change; I'd be more interested in knowing if the lookup is unnecessary:

this.CartItems[this.CartItems.IndexOf(item)].EffectivePrice = 
            CurrencyHelper.GetLocalizedCurrency(item.EffectivePrice);

to:

item.EffectivePrice = CurrencyHelper.GetLocalizedCurrency(item.EffectivePrice);

Other than that, I'm not sure it is worth the bother of changing it; I'd probably leave it as:

foreach (var item in this.CartItems) {
    if (item.EffectivePrice != null) {
        item.EffectivePrice = CurrencyHelper.GetLocalizedCurrency(item.EffectivePrice);
    }
}

Answer 3

In addition to Marc's point, LINQ is more for functional(ish) stuff, and isn't so helpful at mutating existing data structures. Which is a good thing. So if you wanted to produce a new array of new objects, you'd go with something like:

var newItems = CartItems
    .Select(i => CreateNewItemWithPrice(i, item.EffectivePrice ?? 
        CurrencyHelper.GetLocalizedCurrency(item.EffectivePrice))
    .ToList();

In general, that's a very nice approach, since mutating data can cause an awful lot of bugs.

Answer 4

Straight up answer to your question (about how to implement your code in Linq):

this.CartItems.Where(item => item.EffectivePrice != null).ToList().ForEach
(
   item =>
      item.EffectivePrice = CurrencyHelper.GetLocalizedCurrency(item.EffectivePrice);
);

There is no reason to have to explicitly specify the index of the item in the list (at least I haven't seen a reason). The .ToList() gives you a List<T> of object references for you to manage. You AsQueryable() to save a few CPU cycles.

It's a little strange, however, to overwrite a property with the results of the method call as subsequent method calls on that property may alter the value over and over again.

But, nonetheless, Linq's method is far more elegant. The drawback, that I can see, is the inability to edit-and-continue with any method that contains Linq.