-2

I've faced some issue with C# code. Adding method to delegate in "for" loop increments "i" by one., so "for(int i = 0, i < x ; i++)" must be change to "for(int i = -1, i < x-1; i++)" to work correctly. Why is that?

Code below throws an IndexOutOfRangeException

string[] names = new string[] { "John", "Madeline", "Jack", "Gabby" };
Action showNameDelegate = null;
for (int i = 0; i < names.Length; i++)
{
    showNameDelegate += () => global::System.Console.WriteLine(names[i]);
}
foreach (Action showName in showNameDelegate.GetInvocationList())
{
    showName();
}

Right code is (look at iterator "i" which starts from -1 but "names[-1]" does not exist):

string[] names = new string[] { "John", "Madeline", "Jack", "Gabby" };
Action showNameDelegate = null;
for (int i = -1; i < names.Length - 1; i++)
{
    showNameDelegate += () => global::System.Console.WriteLine(names[i]);
}
foreach (Action showName in showNameDelegate.GetInvocationList())
{
    showName();
}

This answer is correct (by Ed Plunkett): Each delegate references the variable i. They all reference the variable, so when they execute, they'll get whatever value it has right then. The delegates are executed after the for loop completes. At that point, i is equal to names.Length. Make a local copy of i in the body of the loop -- or use a foreach loop, which automatically fixes this issue.

Lechoo Duo
  • 1
  • 3
  • 1
    Try making this the contents of your loop: `string name = names[i]; showNameDelegate += () => global::System.Console.WriteLine(name);` – itsme86 Nov 20 '17 at 18:05
  • That’s closures in action. There are many answers to this already within the site. – Ousmane D. Nov 20 '17 at 18:12
  • Each delegate references the variable `i`. They all reference *the variable*, so when they execute, they'll get whatever value it has right then. The delegates are executed *after* the for loop completes. At that point, `i` is equal to `names.Length`. Make a local copy of `i` in the body of the loop -- or use a `foreach` loop, which automatically fixes this issue. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 18:12
  • 1
    It might be ReSharper, but Visual Studio actually warns me with your original code with a squiggly under `i` in `names[i]` and says "Access to modified closure". – itsme86 Nov 20 '17 at 18:13
  • 1
    `foreach` is the better way to iterate through all the items in a collection anyway: `foreach (var name in names) { showNameDelegate += () => global::System.Console.WriteLine(name); }`. `foreach` expresses your intent directly, instead of all this messing around with incrementing `i` and starting at at the right value. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 18:14
  • Your code changes the meaning. If you make a local variable "string name", adding method to delegate change the result. After your change: "foreach (Action showName in showNameDelegate.GetInvocationList()) { showName(); } gives result "John", "Madeline", "Jack", "Gabby". Without making local variable result is like "Gabby","Gabby","Gabby","Gabby" which I expect. – Lechoo Duo Nov 20 '17 at 18:19
  • 1
    Whose code changes what meaning? You say "after your change [results that look correct]". Are you saying you don't want all of the names listed? If you want "Gabby" 4 times, why are you looping through the array? Just print the last name. It's like looping from 1 to 10 to print 10. It doesn't make sense. – itsme86 Nov 20 '17 at 18:21
  • @LechooDuo *"Adding method to delegate in "for" loop increments "i" by one."* -- **Wrong**. It does not and cannot have that effect. If that is part of your theory about what your code is doing, your theory is wrong and you need to start over in your effort to understand what's going on here. I suggest you use the debugger. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 18:22
  • @itsme86 the question is why the iteration is changed? I don't ask how to change my code to work properly. – Lechoo Duo Nov 20 '17 at 18:29
  • 1
    @LechooDuo That's been explained to you more than once, and you refuse to listen. Why should anybody bother explaining it again? – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 18:32

1 Answers1

0

You should change your code like this;

for (int i = -1; i < names.Length - 1; i++)
{
    string name = names[i];
    showNameDelegate += () => global::System.Console.WriteLine(name);
}

Or you can try to use foreach.

lucky
  • 12,734
  • 4
  • 24
  • 46
  • I'd make a variable for `names[i]` instead of just `i`. Look what happens if you, for example, `names[0] = "Felix";` before the second loop that actually prints the names. – itsme86 Nov 20 '17 at 18:16
  • Yes, you have a point. I am updating my answer. – lucky Nov 20 '17 at 18:17
  • I'd simply use `foreach`. Not only does it (since C#5 IIRC) scope the loop variable in the block, thus fixing the problem, it's also a cleaner and more idiomatic way to write that loop in C#. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 18:21
  • Your code changes the meaning. If you make a local variable "string name", adding method to delegate change the result. After your change: "foreach (Action showName in showNameDelegate.GetInvocationList()) { showName(); } gives result "John", "Madeline", "Jack", "Gabby". Without making local variable result is like "Gabby","Gabby","Gabby","Gabby" which I expect. – Lechoo Duo Nov 20 '17 at 18:23
  • 1
    @LechooDuo If you want to print the last item in the list four times, your best bet is to write code that does that and looks like it was *intended* to do that. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 18:24
  • @LechooDuo so if you want to print last item only, you don't need use i variable. Just use like that "showNameDelegate += () => global::System.Console.WriteLine(names[names.Length - 1]);". Actually it comes weird to me. What do you want to do ? – lucky Nov 20 '17 at 18:27
  • @Ed Plunkett: the question is why the iteration is changed? I don't ask how to change my code to work properly. I wrote this code just to demonstrate iteration change. – Lechoo Duo Nov 20 '17 at 18:33
  • @LechooDuo I told you that. Go read my first comment on your question again, as many times as you need to. Also read the question I marked as a duplicate. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 18:34
  • @Ed Plunket You do not understand the question. Read it again and again. – Lechoo Duo Nov 20 '17 at 18:51
  • @LechooDuo You should also re-read my comment above with the word “wrong” in boldface. That one addresses the underlying problem: you can’t understand what we’re telling you because you made up an incorrect story about what the code is doing, and now you refuse to let go of that story. You insist on an answer that fits your story, but your story is 100% wrong, so no correct answer can be consistent with it. This isn’t a problem for me. It’s a problem for you. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 19:03
  • @Ed Plunkett What I mean is: First code I present is wrong. Second code is right. Gives result that I want. Differences between First and Second code are only in loop First: (int i = 0; i < names.Length; i++) Second: (int i = -1; i < names.Length - 1; i++) No other differences!!!! Question: Why First code is wrong? And why Second is right? – Lechoo Duo Nov 20 '17 at 19:11
  • @LechooDuo Once again: This has been explained to you. If you refusde to read the explanation the first time, and you refused to read it when I told you to read it again, you still won't read it if I explain it once more. I can't help you. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 19:14
  • @Ed Plunkett I've added some paragraphs in my question. Please look at it (especially last paragraph). If my question is still hard to understand I'll give up. – Lechoo Duo Nov 20 '17 at 19:49
  • @LechooDuo Use the debugger. The theory of counting from zero to four may be confusing to you, but perhaps you'll begin to understand if you watch it happen in the debugger. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 19:58
  • @Ed Plunkett From 0 to 3 does not work, but from -1 to 2 work. {0,1,2,3} -> not work. {-1,0,1,2} -> work. The same number of elements. names[-1] does not exist but code work properly. I cant belive you still do not understand. – Lechoo Duo Nov 20 '17 at 20:04
  • @LechooDuo I think I've fed the troll more than enough for one day. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 20:10
  • 1
    @Ed Plunkett The debugger was a great idea. Problem was in showName() method. showName() was trying to call Console.WriteLine(names[4]) method but names[4] does not exist. In {-1,0,1,2} version (which is my Second) iteration finishes with 3 where i = names.Length. names[3] exist so everything works fine. This is answer: Each delegate references the variable i. They all reference the variable, so when they execute, they'll get whatever value it has right then. The delegates are executed after the for loop completes. At that point, i is equal to names.Length Thank You Ed Plunkett!!! – Lechoo Duo Nov 20 '17 at 20:37
  • That's exactly correct. It's what we've been trying to tell you for two hours, but you always understand things best if you figure them out yourself. The debugger is much harder to confuse than programmers are -- always step through the code first. – 15ee8f99-57ff-4f92-890c-b56153 Nov 20 '17 at 20:40