Peter Ritchie's MVP Blog : .NET Development, Pop Quiz

ITSWITCH #1: Answer

PeterRitchie — Mon, 28 Jul 2008 19:00:00 GMT

Last post I detailed some code that may or may not have something wrong in it. If you thought InitializeOne and IntializeTwo are semantically identical (e.g. they differ only by performance), you'd be wrong.

If you simply ran the code, you'd be able to guess where the problem is. To understand what's causing the problem. Let's look at how C# effectively implements the two loops.

InitializeOne is essentially equivalent to

        private class PrivateDelegateHelper

            public String Value { get; set; }

            public void Method()

                TestClass.ProcessText(Value);

        public void InitializeThree(String[] strings)

            delegates = new List<MethodInvoker>(strings.Length);

            MethodInvoker cachedAnonymousDelegate = null;

            PrivateDelegateHelper privateDelegateHelper = new PrivateDelegateHelper();

            String[] copyOfStrings = strings;

            for(int i = 0; i < copyOfStrings.Length; ++i)

                privateDelegateHelper.Value = copyOfStrings;

                if (cachedAnonymousDelegate == null)

                    cachedAnonymousDelegate = new MethodInvoker(privateDelegateHelper.Method);

                delegates.Add(cachedAnonymousDelegate);

Now it's obvious, right?

For those you don't want to read all the code, the problem is that only one PrivateDelegateHelper object is instantiated and its value property is set in each iteration of the loop. Because the delegates aren't run until sometime after the loop, they're all run with the last value of the string array as their argument.

The technical term for what we've implemented here is a closure. If you're using Resharper 4.x, you would have noticed a warning "Access to modified closure":

...which is attempting to tell you that the closure (the delegate and cached bound variables) has changed (in this case one of the bound variables has changed between the creation of a closure and another and out expected output is effected).

By the way, you can get the same thing with C# 3+ with lambdas (i.e. you can also write closures with lambdas):

        public void InitializeOne(String[] strings)

            delegates = new List<MethodInvoker>(strings.Length);

            for (int i = 0; i < strings.Length; ++i)

                String value = strings;

                delegates.Add(() => ProcessText(value));

        public void InitializeTwo(String[] strings)

            delegates = new List<MethodInvoker>(strings.Length);

            foreach(String value in strings)

                delegates.Add(() => ProcessText(value));

ITSWITCH: #1

PeterRitchie — Fri, 25 Jul 2008 18:58:00 GMT

Is There Something Wrong In This Code Here

UPDATE: as several readers pointed out there was compile error in the code what was being displayed. The line "String value = string" was showing up as "String value = string". I'm not sure why; but throwing some spaces between the i and the square brackets seems to have solved it.

In the following class, is there something wrong with either InitializeOne or InitializeTwo (hint, it has nothing to do with compile errors and I'm ignoring performance differences)?

    public class TestClass

        private List<MethodInvoker> delegates;

        private static void ProcessText(String text)

            Trace.WriteLine(text);

        public void Run()

            foreach(MethodInvoker invoker in delegates)

                invoker();

        public void InitializeOne(String[] strings)

            delegates = new List<MethodInvoker>(strings.Length);

            for (int i = 0; i < strings.Length; ++i)

                String value = strings[ i ];

                delegates.Add(delegate { ProcessText(value); });

        public void InitializeTwo(String[] strings)

            delegates = new List<MethodInvoker>(strings.Length);

            foreach(String value in strings)

                delegates.Add(delegate { ProcessText(value); });

Answer on Monday.