Is it better coding practice to define variables outside a foreach even though more verbose?

Question

In the following examples:

• the first seems more verbose but less wasteful of resources
• the second is less verbose but more wasteful of resources (redefines string each loop)

Which is better coding practice?

First example:

using System;
using System.Collections.Generic;

namespace TestForeach23434
{
    class Program
    {
        static void Main(string[] args)
        {
            List<string> names = new List<string> { "one", "two", "two", "three", "four", "four" };

            string test1 = "";
            string test2 = "";
            string test3 = "";
            foreach (var name in names)
            {
                test1 = name + "1";
                test2 = name + "2";
                test3 = name + "3";
                Console.WriteLine("{0}, {1}, {2}", test1, test2, test3);
            }
            Console.ReadLine();
        }
    }
}

Second example:

using System;
using System.Collections.Generic;

namespace TestForeach23434
{
    class Program
    {
        static void Main(string[] args)
        {
            List<string> names = new List<string> { "one", "two", "two", "three", "four", "four" };

            foreach (var name in names)
            {
                string test1 = name + "1";
                string test2 = name + "2";
                string test3 = name + "3";
                Console.WriteLine("{0}, {1}, {2}", test1, test2, test3);
            }
            Console.ReadLine();
        }
    }
}

Answer 1

The second form is no more wasteful - it's simply better.

There's no advantage to declaring the variables outside the loop, unless you want to maintain their values between iterations.

(Note that usually this makes no behavioural difference, but that's not true if the variables are being captured by a lambda expression or anonymous method.)

Answer 2

Personally, I think it's the best practice to declare variables in the tightest scope possible, given their usage.

This provides many benefits:

1. It's easier for refactoring, since extracting a method is simpler when the variables are already in the same scope.
2. The variable usage is more clear, which will lead to more reliable code.

The only (potential) disadvantage would be the extra variable declaration - however, the JIT tends to optimize this issue away, so it's one I wouldn't necessarily worry about in real work.

The one exception to this:

If your variable is going to be adding a lot of GC pressure, and if this can be avoided by reusing the same object instance through the foreach/for loop, and if the GC pressure is causing measured performance problems, I'd lift it into the outer scope.

Answer 3

Those are both wasteful and verbose.

foreach (var name in names)
{
   Console.WriteLine("{0}1, {0}2, {0}3", name);
}

.

</tongueincheek>

Answer 4

Depending on language and compiler it may or may not be the same. For C# I expect the resulting code to be very similar.

My own philosophy on this is simple:

Optimize for ease of understanding.

Anything else is premature optimization! The biggest bottleneck in most development is time and attention of the developer. If you absolutely must squeeze out every last CPU cycle then by all means do so, but unless you have a compelling business need to do or are writing a critical component (common library, operating system kernel, etc) you are better off waiting until you can benchmark the finished program. At that time optimizing a few of the most costly routines is justified, before then it's almost certainly a waste of time.

Answer 5

I'm not sure what you gain by defining the string variable outside of the loop. Strings are immutable so they are not reused. Whenever you assign to them, a new instance is created.

Answer 6

I think it depends on what you are trying to solve. I like the second example, because you can move the code in one step. I like the first example, because it is faster due to less stack manipulations, less memory fragmentation, and less object construction/creation.

Answer 7

I've found that 'hoisting' declarations out of loops is typically a better long-term strategy for maintenance. The compiler will usually sort things out acceptably for performance.

Answer 8

They are both virtually the same as far as performance goes (strings are immutable), but as for readability... I'd say neither is very good. You could easily just do it all within the Console.WriteLine.

Perhaps you can post the real problem instead of an example?

Answer 9

I generally declare variables as close to their use as scoping allows, which in this case would be your second example. Resharper tends to encourage this style as well.

Answer 10

For POD type data declare closest to first use. For anything like a class that does any memory allocation, then you should consider declaring those outside of any loops. Strings will almost certainly do some form of allocation and most implementations (at least in C++) will attempt to reuse memory if possible. Heap based allocations can be very slow indeed.

I once profiled a bit of C++ code that included a class that new'd data in its ctor. With the variable declared outside of the loop it ran 17% faster than with the variable declared inside the loop. YMMV in C# so profile performance you may be very surprised at the results.

Answer 11

This is my favourite part of Linq which I guess it fits here:

names.ForEach(x => Console.WriteLine("{0}1, {0}2, {0}3", x));

Answer 12

Follow a simple rule while declaring variables

Declare it when you need it for the first time