Search a list containing class objects and remove the class object that matches an instance variable of class object

Question

This is in Python 3.3.

So I have two classes: Employee and Zone. Zone takes the Employee objects I create and puts their name and ID into a list. I want the remove_employee method to take an employee's ID in a string, find the employee that matches the ID, and remove them from the list. Here's what I have so far:

class Employee:
employee_count = 0

    def __init__(self, name, employee_id):
        self._name = name
        self._employee_id = employee_id
        Employee.employee_count += 1

    def get_employee_id(self):
        return self._employee_id


class Zone:

    def __init__(self, name="Kitchen", employees=[]):
        self._name = name
        self._employees = employees

    def add_employee(self, employee):
        self._employees += [employee]

    def remove_employee(self, employee_id):
        for employee_id in self._employees:
            self._employees.remove(employee_id)

    def number_of_employees(self):
        return len(self._employees)

And when I try to run this:

John = Employee('John', 'F12345')
Jack = Employee('Jack', 'F23514')
Jane = Employee('Jane', 'F10253')
Kitchen = Zone()
Kitchen.add_employee(John)
Kitchen.add_employee(Jack)
Kitchen.add_employee(Jane)
Kitchen.remove_employee('F12345') #John's ID

It removes John and Jane from the list, instead of just John. I'm very new to programming and I'm completely stumped by how to write remove_method().

Any help would be greatly appreciated!

Not the immediate problem, but watch out for that [mutable default argument](http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument) on `Zone.__init__`. — user2357112, Nov 08 '15 at 22:30

Daniel Roseman · Answer 1 · 2015-11-08T22:43:15.703

In remove_employee, you don't actually check that the employee ID matches the current value in your loop. Also, you shouldn't modify a list you're iterating over; it can cause all sorts of problems. Seems like you intend something like this:

def remove_employee(self, employee_id):
    employees = []
    for employee in self._employees:
        if employee._employee_id != employee_id:
            employees.append(employee)
    self._employees = employees

which can be significantly shortened with something called a list comprehension:

def remove_employee(self, employee_id):
    self._employees = [e for e in self._employees if e._employee_id != employee_id]

However, the whole thing would be even simpler if you used the appropriate data structure to store employees, which is a dict, keyed by ID, not a list. For example:

class Zone:
    def __init__(self, name="Kitchen", employees=None):
        self._name = name
        self._employees = employees or {}

    def add_employee(self, employee):
        self._employees[employee._employee_id] = employee

    def remove_employee(self, employee_id):
        self._employees.pop(employee_id)

    def number_of_employees(self):
        return len(self._employees)

I think these solutions incurs overhead from creating/appending an additional list. Why not modify the list in-line? — mattsap, Nov 08 '15 at 22:45
For the reason I gave: you shouldn't modify a list you're iterating over. See [this answer](http://stackoverflow.com/a/6260097/104349) for a good explanation of why. — Daniel Roseman, Nov 08 '15 at 22:48
@mattsap: Such small lists aren't going to have a noticeable impact on RAM usage. However, it may be desirable to modify the existing `self._employees` list, rather than replacing it. And that's easily accomplished via slice assignment: `self._employees[:] = employees`. — PM 2Ring, Nov 08 '15 at 22:53

score 0 · Accepted Answer · answered Nov 08 '15 at 22:41

The issue is you are iterating over the employees list with

for employee_id in self._employees

and removing the employees.

You should change do something like this instead:

def remove_employee(self, employee_id):
    for employee in self._employees:
        if employee.get_employee_id() == employee_id:
            self._employees.remove(employee)

or better yet filter! =)

def remove_employee(self, employee_id):
        self.employees = list(filter(lambda x: x.get_employee_id() != employee_id))

Search a list containing class objects and remove the class object that matches an instance variable of class object

2 Answers2