Code isn't iterating over the whole Dictionary?

Question

I am stumbling upon a problem, and I have no clue about what I'm doing wrong. I tried countless different things, but for some reason it just won't work. My main loop:

static Dictionary<string, int> dict = new Dictionary<string, int>();

public static void IterateOverEachUser()
{
    if (dict.Count > 0) {
        foreach (KeyValuePair<string, int> item in dict.ToList())
        {
            string userName = item.Key;
            int amountLeft = item.Value;
            if(amountLeft == 60)
            {
                Log(userName + " started!");
            }
            Log(userName + amountLeft);
            dict[userName] = dict[userName] - 1;
            amountLeft = item.Value;
            if(amountLeft == 0)
            {
                Log(userName + " ran out!");
            }
        }
    }
}

public static void AddUser(string User)
{
    if (dict.ContainsKey(User))
    {
        Log("User already exists.");
    }
    else
    {
        dict.Add(User,60);
        Log("User has been added.");
    }
}

I am looping IterateOverEachUser() every 5 seconds. When I add a user using the method, everything is fine, but when I add a second one, his value is stuck at 60 while the other one keeps rolling.

Does anyone knows why this happens? I'm coming from Java using HashMaps and using the same code there works as intended. (Which is: every user gets iterated over, the value of all users get deducted by 1 and then it stops until the IterateOverEachUser() method gets called again by the 5 second loop).

Thanks in advance!

I get downvoted but I don't get any constructive criticism? As far as I am concerned, my code should work perfectly, right? I posted my own code, I showed I did extensive research, I explained my problem and I explain what I expect the code to do. This does not deserve a downvote.. imo — Dysanix Official, May 24 '16 at 05:25
be careful, you're modifying a dictionary while looping over it. In most language, this can be an issue — Hoàng Long, May 24 '16 at 05:30
@HoàngLong Thanks for your feedback! I thought about this as well, but adding ".ToList()" does not solve the problem unfortunately. Also, would you know a better way to do this to maybe circumvent having to edit the dictionary while it's running? — Dysanix Official, May 24 '16 at 05:31
@DysanixOfficial: be careful with ToList as well. The List may be a new one, but its member may still refer to the old KeyValuePair objects. The way to make sure is that you copy your Dictionary to a new list, then modify there — Hoàng Long, May 24 '16 at 05:34
@DysanixOfficial Why are you parsing the Dictionary to a List? I think you can iterate over the dictionary without parsing. — Marius, May 24 '16 at 05:37
Adding ToList() does solve the problem. It is another List object which is enumerated, not the dictionary. — Tomas Kubes, May 24 '16 at 05:37
The code itself works fine ([this dotNetFiddle](https://dotnetfiddle.net/NxMWwK)). Which is exactly why you should provide a [mcve]. Your problem is somewhere else. — Manfred Radlwimmer, May 24 '16 at 05:52
See that we have an answer :) Just to clarify the point that ToList() not always create a list of new objects: http://stackoverflow.com/questions/2774099/tolist-does-it-create-a-new-list . Maybe here KeyValuePair is more like a struct than a class, so we don't have this problem — Hoàng Long, May 24 '16 at 06:01

score 4 · Accepted Answer · answered May 24 '16 at 05:45

amountLeft = item.Value; will produce wrong values (the old one).

Also, when working with timers and static resources, it is better to make your method thread-safe using lock.

The below test code works perfectly for me:

static Dictionary<string, int> dict = new Dictionary<string, int>();
static object lockObject = new Object();

public static void IterateOverEachUser()
{
    lock (lockObject)
    {
        if (dict.Count > 0)
        {
            foreach (KeyValuePair<string, int> item in dict.ToList())
            {
                string userName = item.Key;
                int amountLeft = item.Value;
                if (amountLeft == 60)
                {
                    Console.WriteLine(userName + " started!");
                }
                Console.WriteLine(userName + amountLeft);
                dict[userName] = dict[userName] - 1;
                amountLeft = dict[userName];
                if (amountLeft == 0)
                {
                    Console.WriteLine(userName + " ran out!");
                }

                Console.WriteLine("User " + item.Key + " = " + amountLeft);
            }
        }
    }
}

public static void AddUser(string User)
{
    if (dict.ContainsKey(User))
    {
        Console.WriteLine("User already exists.");
    }
    else
    {
        dict.Add(User, 60);
        Console.WriteLine("User has been added.");
    }
}

static void Main(string[] args)
{

    AddUser("U1");
    AddUser("U2");

    int counter = 1;
    System.Timers.Timer t1 = new System.Timers.Timer();
    t1.Interval = 5000;
    t1.Elapsed += (oo, ee) => 
    {
        IterateOverEachUser();

        if (counter++ == 5)
            AddUser("U3");
    };

    t1.Start();

    Console.ReadKey();
}

Output:

User has been added.
User has been added.
U1 started!
U160
User U1 = 59
U2 started!
U260
User U2 = 59
U159
User U1 = 58
U259
User U2 = 58
U158
User U1 = 57
U258
User U2 = 57
U157
User U1 = 56
U257
User U2 = 56
U156
User U1 = 55
U256
User U2 = 55
User has been added.
U155
User U1 = 54
U255
User U2 = 54
U3 started!
U360
User U3 = 59
U154
User U1 = 53
U254
User U2 = 53
U359
User U3 = 58
U153
User U1 = 52
U253
User U2 = 52
U358
User U3 = 57
// and so one

Locking the object worked perfectly! Guess I'll have to dig back into the documentation and learn more about threading. Thanks so much for your answer, and thanks everybody else who contributed as well! — Dysanix Official, May 24 '16 at 05:53

Code isn't iterating over the whole Dictionary?

1 Answers1