views:

396

answers:

7

Are there advantages to either approach? If I need to traverse List items and perform an action on each one, should I use the traditional foreach loop mechanism or move on to List.ForEach?

Matthew Podwysocki @ CodeBetter.com wrote an interesting article about the anti-for campaign. This got me thinking about the problem a loop is trying to solve. In this article, Matthew argues that explicit loop structures make you think about the 'how' instead of the 'what'.

What are some good reasons to use one over the other (if there are any)?

+2  A: 

I only see an advantage if you have an existing delegate you want to pass to .ForEach().

In most other cases, I think using a real foreach() is more efficient and more readable.

Philippe Leybaert
Really? See CIL output below and performance benchmark.
Robert Venables
I doubt the accuracy of these benchmarks. The sample is too small and the test-case is not representative of real world code (IMHO)
Philippe Leybaert
Updated with more realistic code. The List.ForEach method is still significantly faster. If you have any suggestions to make the test more valid please let me know.
Robert Venables
(Also increased the number of actual iterations per test and ran it four times)
Robert Venables
+1  A: 

I agree with Matthew Podwysocki. Unless you're passing delegates around and want to loop over a collection using one of them, I'd stick to standard loop constructs.

Ben M
+8  A: 

For one thing, you'd use it if you'd been passed the delegate to apply for whatever reason. For example, you might create your own list, populate it etc and then apply a delegate to each entry. At that point, writing:

list.ForEach(action);

is simpler than

foreach (Item item in list)
{
    action(item);
}
Jon Skeet
+1  A: 

Eric Lippert has a good blog post about this very subject here

thecoop
+4  A: 

I found List.ForEach to be significantly faster. Here are the results of the last four runs of the (now revised) performance test:

NativeForLoop:        00:00:04.7000000
ListDotForEach:       00:00:02.7160000
---------------------------------------
NativeForLoop:        00:00:04.8660000
ListDotForEach:       00:00:02.6560000
---------------------------------------
NativeForLoop:        00:00:04.6240000
ListDotForEach:       00:00:02.8160000
---------------------------------------
NativeForLoop:        00:00:04.7110000
ListDotForEach:       00:00:02.7190000

Each test was executed with one hundred million (100,000,000) iterations. I updated the test to use a custom class (Fruit) and have each loop access and work with a member inside the current object. Each loop is performing the same task.

Here is the entire source of the test class:

class ForEachVsClass
{

static Int32 Iterations = 1000000000;
static int Work = 0;

public static void Init(string[] args)
{
    if (args.Length > 0)
        Iterations = Int32.Parse(args[0]);
    Console.WriteLine("Iterations: " + Iterations);
}

static List<Fruit> ListOfFruit = new List<Fruit> { 
    new Fruit("Apple",1), new Fruit("Orange",2), new Fruit("Kiwi",3), new Fruit("Banana",4) };


internal class Fruit {
    public string Name { get; set; }
    public int Value { get; set; }
    public Fruit(string _Name, int _Value) { Name = _Name; Value = _Value; }
}


[Benchmark]
public static void NativeForLoop()
{
    for (int x = 0; x < Iterations; x++)
    {
        NativeForLoopWork();
    }

}

public static void NativeForLoopWork()
{
    foreach (Fruit CurrentFruit in ListOfFruit) {
        Work+=CurrentFruit.Value;
    }
}


[Benchmark]
public static void ListDotForEach()
{
    for (int x = 0; x < Iterations; x++)
    {
        ListDotForEachWork();
    }
}

public static void ListDotForEachWork()
{
    ListOfFruit.ForEach((f)=>Work+=f.Value);
}

}

Here is the resulting IL for the work methods (extracted to make them easier to read):

.method public hidebysig static void NativeForLoopWork() cil managed
{
    .maxstack 2
    .locals init (
        [0] class ForEachVsClass/Fruit CurrentFruit,
        [1] valuetype [mscorlib]System.Collections.Generic.List`1/Enumerator<class ForEachVsClass/Fruit> CS$5$0000)
    L_0000: ldsfld class [mscorlib]System.Collections.Generic.List`1<class ForEachVsClass/Fruit> ForEachVsClass::ListOfFruit
    L_0005: callvirt instance valuetype [mscorlib]System.Collections.Generic.List`1/Enumerator<!0> [mscorlib]System.Collections.Generic.List`1<class ForEachVsClass/Fruit>::GetEnumerator()
    L_000a: stloc.1 
    L_000b: br.s L_0026
    L_000d: ldloca.s CS$5$0000
    L_000f: call instance !0 [mscorlib]System.Collections.Generic.List`1/Enumerator<class ForEachVsClass/Fruit>::get_Current()
    L_0014: stloc.0 
    L_0015: ldsfld int32 ForEachVsClass::Work
    L_001a: ldloc.0 
    L_001b: callvirt instance int32 ForEachVsClass/Fruit::get_Value()
    L_0020: add 
    L_0021: stsfld int32 ForEachVsClass::Work
    L_0026: ldloca.s CS$5$0000
    L_0028: call instance bool [mscorlib]System.Collections.Generic.List`1/Enumerator<class ForEachVsClass/Fruit>::MoveNext()
    L_002d: brtrue.s L_000d
    L_002f: leave.s L_003f
    L_0031: ldloca.s CS$5$0000
    L_0033: constrained [mscorlib]System.Collections.Generic.List`1/Enumerator<class ForEachVsClass/Fruit>
    L_0039: callvirt instance void [mscorlib]System.IDisposable::Dispose()
    L_003e: endfinally 
    L_003f: ret 
    .try L_000b to L_0031 finally handler L_0031 to L_003f
}



.method public hidebysig static void ListDotForEachWork() cil managed
{
    .maxstack 8
    L_0000: ldsfld class [mscorlib]System.Collections.Generic.List`1<class ForEachVsClass/Fruit> ForEachVsClass::ListOfFruit
    L_0005: ldsfld class [mscorlib]System.Action`1<class ForEachVsClass/Fruit> ForEachVsClass::CS$<>9__CachedAnonymousMethodDelegate1
    L_000a: brtrue.s L_001d
    L_000c: ldnull 
    L_000d: ldftn void ForEachVsClass::<ListDotForEachWork>b__0(class ForEachVsClass/Fruit)
    L_0013: newobj instance void [mscorlib]System.Action`1<class ForEachVsClass/Fruit>::.ctor(object, native int)
    L_0018: stsfld class [mscorlib]System.Action`1<class ForEachVsClass/Fruit> ForEachVsClass::CS$<>9__CachedAnonymousMethodDelegate1
    L_001d: ldsfld class [mscorlib]System.Action`1<class ForEachVsClass/Fruit> ForEachVsClass::CS$<>9__CachedAnonymousMethodDelegate1
    L_0022: callvirt instance void [mscorlib]System.Collections.Generic.List`1<class ForEachVsClass/Fruit>::ForEach(class [mscorlib]System.Action`1<!0>)
    L_0027: ret 
}
Robert Venables
I'm curious what the result would be for larger list. The list only contains 4 items. (btw: thanks for changing the test case)
Philippe Leybaert
+2  A: 

Eric Lippert has come out against IEnumerable.ForEach() and I can see both sides of the argument. Having pushed his argument against it aside and implemented it, I've found some small bit of glee in how terse and readable it's made a few code blocks.

Having been bitten by side effects that I normally don't have to think about with LINQ, I can also see why he made the case for not shipping with it.

The case with delegates is a stronger one for ForEach() but I don't think a standard foreach loop is obscuring intent all that much.

I don't think there's any definitively right or wrong answer.

48klocs
A: 

Sometimes I find that I can right easier-to-read code in a Lambda expression in .ForEach(). It also keeps you from having to explicitly write out the type you are iterating over. Since it is strongly typed, the compiler already knows.

Example:

logs.ForEach(log =>
    {
        log.DoSomething();
    });
mc2thaH
Agreed on the lambda expression part, but nothing stops you from writing: foreach (var log in logs)
Steven Sudit
true. just two ways to do one thing. But you can also use a delegate, and it does become less code.
mc2thaH