Why does this code only work half the time?

Question

I have the following test

def test_employees_not_arround_for_more_than_3_rounds(self):
    self.game_object.generate_workers()

    people_in_list_turn_1 = self.game_object.employees[:]
    self.game_object.next_turn()
    self.game_object.generate_workers()
    self.game_object.next_turn()
    self.game_object.generate_workers()
    self.game_object.next_turn()

    for employee in people_in_list_turn_1:
        self.assertFalse(employee in self.game_object.employees)

Basically, it generates a random number of workers and adds this to my game_object.employees list. When I call the game_object.next_turn function, every employee has a turns_unemployed variable that holds the number of turns they have been unemployed, once this reaches 3, the worker will be removed from the game_object.employees list altogether.

Here follows the implementation code from game_object.py:

def generate_workers(self):
    workersToAdd = range(random.randrange(1,8))
    for i in workersToAdd:
        self.__employees.append(Employee())

def next_turn(self):
    self.__current_turn += 1
    self.__call_employees_turn_function()
    self.__remove_workers_unemployed_for_3_rounds()

def __call_employees_turn_function(self):
    for employee in self.employees:
        employee.turn()

def __remove_workers_unemployed_for_3_rounds(self):
    for employee in self.employees:
        if employee.turns_unemployed >= 3:
            self.employees.remove(employee)

I already have a test that checks that the turns_unemployed variable is actually increased by one when employee.turn() is called, so I know that works...

The thing that really bugs me here is that my test only works 50% of the time i run it, and I can’t figure out why... Anyone see anything that can cause any discrepancies?

Btw, running Python 3.2.2

Answer 1

You are removing items from a list while iterating over it in __remove_workers_unemployed_for_3_rounds, so the loop skips items you would want it to remove. You need to iterate over a copy of the list.

def __remove_workers_unemployed_for_3_rounds(self):
    for employee in self.employees[:]:
        if employee.turns_unemployed >= 3:
            self.employees.remove(employee)

Example:

You generate 2 new employees on each turn. On the 4th turn, you have 2 employees to remove (the two first in the list). You begin iterating and remove the first one. The list has only five items now, but iterating goes on and looks at the second item. The problem is that the second item is not the second employee anymore, but the third. The second employee will remain in the list and your test will fail. Your test only works if only one employee is generated on the first turn.

Answer 2

Hugo is probably right about what's causing your problem; you can't remove items from a list while you're iterating over it. Here's another possible problem though, when you create employees, you put them in a list called __employees, i.e.

def generate_workers(self):
    workersToAdd = range(random.randrange(1,8))
    for i in workersToAdd:
        self.__employees.append(Employee())

but when you iterate over them later, you're using a list called employees, i.e.

def __call_employees_turn_function(self):
    for employee in self.employees:
        employee.turn()

def __remove_workers_unemployed_for_3_rounds(self):
    for employee in self.employees:
        if employee.turns_unemployed >= 3:
            self.employees.remove(employee)

But I don't know if this is related to your problem because I can't see the rest of your code - I'm not even sure if these are in the same class or not. You should probably post the smallest complete piece of code you can get which has the problem -- that way people can actually run your code and reproduce the issue for themselves.