views:

251

answers:

5

I end up with a lot of code like this:

List<string> dates = someMethodCall();
foreach (string dateStr in dates) { }

I usually declare the object over which I'm iterating and then use it in the foreach condition out of worry that someMethodCall() would happen for each iteration of the loop. Is this the case? I would prefer to do this:

foreach (string dateStr in someMethodCall()) { }

But I only want to do that if someMethodCall() happens only once and then its results are cached for each subsequent iteration.

+12  A: 

The method will be called only once in both cases.

The first method has a readability advantage as you can name the variable and describe what's in it with its name. It will make the code more self-documenting and improves maintainability.


To quote the authoritative source on this:

C# Language Specification - 8.8.4 The foreach statement

 foreach (V v in x) embedded-statement

is then expanded to:

{
  E e = ((C)(x)).GetEnumerator();
  try {
  V v;
      while (e.MoveNext()) {
          v = (V)(T)e.Current;
          embedded-statement
      }
  }
  finally {
      … // Dispose e
  }
}

It's clear that the expression x in the above foreach statement is evaluated only once in the expansion.

Mehrdad Afshari
This first method also makes debugging easier.
Dan Rigby
Who needs the var when you name the method call with enough detail?
ChaosPandion
@Chaos: The method call might have some arguments and then using `var` might make sense as it can describe what is actually in this specific return value. If it's descriptive enough, you can go with the second case.
Mehrdad Afshari
+8  A: 

foreach will evaluate the collection once, get the iterator, and then use that for its iteration.

Anon.
+1  A: 

Also, I'm not sure what your use case is, but if you're worried about the amount of code, lambdas can help clean up in some instances.

For example, if you're writing foreach statements to simply look for particular list elements, consider using the .Where lambda. I've found that using them, when appropriate, has decreased the amount of code I've written and made it more readable in certain situations.

Bit Destroyer
A: 

I'm not the best at reading MSIL, but I did some testing and it seems to agree with what everyone is saying: the collection is only retrieved one time. See below for the MSIL if you're curious.

public static void ATest() {
    foreach (string s in GetSomeStrings()) {
        Console.WriteLine(s);
    }
}
public static void BTest() {
    string[] strings = GetSomeStrings();

    foreach (string s in strings) {
        Console.WriteLine(s);
    }
}
public static string[] GetSomeStrings() {
    return new string[] {
        "string1", "string2", "string3"
    };
}

ATest() MSIL:

.method public hidebysig static void ATest() cil managed
{
    .maxstack 2
    .locals init (
        [0] string s,
        [1] string[] CS$6$0000,
        [2] int32 CS$7$0001,
        [3] bool CS$4$0002)
    L_0000: nop 
    L_0001: nop 
--->L_0002: call string[] EdProgAppData_BLL.Common::GetSomeStrings()
    L_0007: stloc.1 
    L_0008: ldc.i4.0 
    L_0009: stloc.2 
    L_000a: br.s L_001d
    L_000c: ldloc.1 
    L_000d: ldloc.2 
    L_000e: ldelem.ref 
    L_000f: stloc.0 
    L_0010: nop 
    L_0011: ldloc.0 
    L_0012: call void [mscorlib]System.Console::WriteLine(string)
    L_0017: nop 
    L_0018: nop 
    L_0019: ldloc.2 
    L_001a: ldc.i4.1 
    L_001b: add 
    L_001c: stloc.2 
    L_001d: ldloc.2 
    L_001e: ldloc.1 
    L_001f: ldlen 
    L_0020: conv.i4 
    L_0021: clt 
    L_0023: stloc.3 
    L_0024: ldloc.3 
    L_0025: brtrue.s L_000c
    L_0027: ret 
}

BTest() MSIL:

.method public hidebysig static void BTest() cil managed
{
    .maxstack 2
    .locals init (
        [0] string[] strings,
        [1] string s,
        [2] string[] CS$6$0000,
        [3] int32 CS$7$0001,
        [4] bool CS$4$0002)
    L_0000: nop 
--->L_0001: call string[] EdProgAppData_BLL.Common::GetSomeStrings()
    L_0006: stloc.0 
    L_0007: nop 
    L_0008: ldloc.0 
    L_0009: stloc.2 
    L_000a: ldc.i4.0 
    L_000b: stloc.3 
    L_000c: br.s L_001f
    L_000e: ldloc.2 
    L_000f: ldloc.3 
    L_0010: ldelem.ref 
    L_0011: stloc.1 
    L_0012: nop 
    L_0013: ldloc.1 
    L_0014: call void [mscorlib]System.Console::WriteLine(string)
    L_0019: nop 
    L_001a: nop 
    L_001b: ldloc.3 
    L_001c: ldc.i4.1 
    L_001d: add 
    L_001e: stloc.3 
    L_001f: ldloc.3 
    L_0020: ldloc.2 
    L_0021: ldlen 
    L_0022: conv.i4 
    L_0023: clt 
    L_0025: stloc.s CS$4$0002
    L_0027: ldloc.s CS$4$0002
    L_0029: brtrue.s L_000e
    L_002b: ret 
}
Jagd
+1  A: 

One way to remember how this works it to consider this: The iterator wouldn't work if it kept calling your method over and over.

Your method returns a list of items. If the loop kept calling your method over and over, it would (barring side effects) keep getting back that same list. How would the loop know, on the second call, that it had already processed the first item in the list?

Anything you can enumerate over has a GetEnumerator() method, which must return a type (usually a type implementing IEnumerator, but it doesn't have to be). The returned type must have a Current property and a MoveNext() method.

The returned type is your enumerator object, and your foreach loop holds a reference to that enumerator object as it enumerates. It keeps calling Current and MoveNext() on that enumerator object until MoveNext() returns false.

Using foreach is usually more readable and convenient, but you can also enumerate "manually" if you want to:

List<string> dates = someMethodCall();
IEnumerator<string> myEnumerator = dates.GetEnumerator();
while (myEnumerator.MoveNext())
{
    // do something with myEnumerator.Current
}
Kyralessa