Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Please review unique_list:

unique_list implements a list where all items are unique. Functionality can also be described as set with order. unique_list should behave as a python list except:

  • Adding items the end of the list (by append, extend) will do nothing if the item is already in the list.
  • Assigning to the middle of the list (insert, __setitem__) will remove previous item with the same value - if any.

class unique_list(list):
    def __init__(self, initial_list = ()):
        super(unique_list, self).__init__()
        self.attendance = set()
        self.extend(initial_list)
    def __setitem__(self, index, item):
        prev_item = self[index]
        if prev_item != item:
            if item in self.attendance:
                prev_index_for_item = self.index(item)
                super(unique_list, self).__setitem__(index, item)
                del self[prev_index_for_item]
                self.attendance.add(item)
            else:
                super(unique_list, self).__setitem__(index, item)
                self.attendance.add(item)
    def __delitem__(self, index):
        super(unique_list, self).__delitem__(index)
        self.attendance.remove(self[index])
    def __contains__(self, item):
        """ Overriding __contains__ is not required - just more efficient """
        return item in self.attendance
    def append(self, item):
        if item not in self.attendance:
            super(unique_list, self).append(item)
            self.attendance.add(item)
    def extend(self, items = ()):
        for item in items:
            if item not in self.attendance:
                super(unique_list, self).append(item)
                self.attendance.add(item)
    def insert(self, index, item):
        if item in self.attendance:
            prev_index_for_item = self.index(item)
            if index != prev_index_for_item:
                super(unique_list, self).insert(index, item)
                if prev_index_for_item < index:
                    super(unique_list, self).__delitem__(prev_index_for_item)
                else:
                    super(unique_list, self).__delitem__(prev_index_for_item+1)
        else:
            super(unique_list, self).insert(index, item)
            self.attendance.add(item)
    def remove(self, item):
        if item in self.attendance:
            super(unique_list, self).remove(item)
            self.attendance.remove(item)
    def pop(self, index=-1):
        self.attendance.remove(self[index])
        return super(unique_list, self).pop(index)
    def count(self, item):
        """ Overriding count is not required - just more efficient """
        return self.attendance.count(item)

Here is my final (as of now) version:

class unique_list(list):
    __slots__ = ('__attendance',)
    def __init__(self, initial_list = ()):
        super(unique_list, self).__init__()
        self.__attendance = set()
        self.extend(initial_list)
    def __setitem__(self, index, item):
        prev_item = self[index]
        if prev_item != item:
            if item in self.__attendance:
                prev_index_for_item = self.index(item)
                super(unique_list, self).__setitem__(index, item)
                del self[prev_index_for_item]
                self.__attendance.add(item)
            else:
                super(unique_list, self).__setitem__(index, item)
                self.__attendance.remove(prev_item)
                self.__attendance.add(item)
    def __delitem__(self, index):
        super(unique_list, self).__delitem__(index)
        self.__attendance.remove(self[index])
    def __contains__(self, item):
        """ Overriding __contains__ is not required - just more efficient """
        return item in self.__attendance
    def append(self, item):
        if item not in self.__attendance:
            super(unique_list, self).append(item)
            self.__attendance.add(item)

    def extend(self, items = ()):
        for item in items:
            if item not in self.__attendance:
                super(unique_list, self).append(item)
                self.__attendance.add(item)
    def insert(self, index, item):
        if item in self.__attendance:
            prev_index_for_item = self.index(item)
            if index != prev_index_for_item:
                super(unique_list, self).insert(index, item)
                if prev_index_for_item < index:
                    super(unique_list, self).__delitem__(prev_index_for_item)
                else:
                    super(unique_list, self).__delitem__(prev_index_for_item+1)
        else:
            super(unique_list, self).insert(index, item)
            self.__attendance.add(item)
    def remove(self, item):
        if item in self.__attendance:
            super(unique_list, self).remove(item)
            self.__attendance.remove(item)
    def pop(self, index=-1):
        self.__attendance.remove(self[index])
        return super(unique_list, self).pop(index)
    def count(self, item):
        """ Overriding count is not required - just more efficient """
        return self.__attendance.count(item)
share|improve this question
    
Not sure why but the code is cut half way through. Maybe it's too long? Is there a way to make sure all the code is displayed? –  Periodic Maintenance Feb 25 '13 at 15:56
1  
You'll find it worth comparing your implementation with the OrderedSet recipe. –  Gareth Rees Feb 25 '13 at 20:34

1 Answer 1

up vote 2 down vote accepted
class unique_list(list):

I recommend not inheriting from list. You don't gain a whole lot from it as you need to overload pretty much all the methods anyways. By inheriting from list you might also get some methods you didn't think of or were added later and thus behave incorrectly because you should have overloaded them. I'd suggest having the backing list as an attribute instead of the base class.

    def __init__(self, *args):

Why are you taking a variable number of arguments? That's not how list does it, and so why are you deviating here?

        super(unique_list, self).__init__()
        self.attendance = set()
        self.extend(args)

    def __setitem__(self, index, item):
        prev_item = self[index]
        if prev_item != item:
            if item in self.attendance:
                prev_index_for_item = self.index(item)
                super(unique_list, self).__setitem__(index, item)
                del self[prev_index_for_item]
            else:
                super(unique_list, self).__setitem__(index, item)
                self.attendance.add(item)

Don't you need to update the self.attendance for the object you are overwriting?

    def __delitem__(self, index):
        super(unique_list, self).__delitem__(index)
        self.attendance.remove(self[index])
    def __contains__(self, item):
        """ Overriding __contains__ is not required - just more efficient """
        return item in self.attendance
    def append(self, item):
        if item not in self.attendance:
            super(unique_list, self).append(item)
            self.attendance.add(item)

In the other cases, new items take precedence over old items. But here, you are keeping the old item, and not appending the new item.

    def extend(self, items = ()):

Why is there a default here? The function is no-op with the default.

        for item in items:
            if item not in self.attendance:
                super(unique_list, self).append(item)
                self.attendance.add(item)

Ok, so you are deliberately keeping old items when appending to the end, and new items when setting? That just seems inconsistent and surprising.

    def insert(self, index, item):
        if item in self.attendance:
            prev_index_for_item = self.index(item)
            if index != prev_index_for_item:
                super(unique_list, self).insert(index, item)
                if prev_index_for_item < index:
                    del self[prev_index_for_item]
                else:
                    del self[prev_index_for_item+1]
        else:
            super(unique_list, self).insert(index, item)
            self.attendance.add(item)

I'd suggest that you simplify this logic by breaking it into parts:

def insert(self, index, item):
    self.remove(item)
    super(self, unique_list).insert(index, item)
    self.attendance.add(item)

That way your code will be much clearer. I'd write all your functions in that manner and only deviate after performance was shown to be a concern.

    def remove(self, item):
        if item in self.attendance:
            super(unique_list, self).remove(item)
            self.attendance.remove(item)
    def pop(self, index=None):
        if index is None:
            index = len(self.attendance) - 1

Can you get away with defaulting to -1?

        self.attendance.remove(self[index])
        return super(unique_list, self).pop(index)
    def count(self, item):
        """ Overriding count is not required - just more efficient """
        return self.attendance.count(item)
share|improve this answer
    
"In the other cases, new items take precedence over old items. But here, you are keeping the old item, and not appending the new item." The idea is that appended items already in the list will keep their place. However if the programmers wants to explicitly make changes to the order - they will be able to do so using [] or insert. –  Periodic Maintenance Feb 25 '13 at 17:06
    
def insert: If I self.remove(item) before super(self, unique_list).insert(index, item) the index might not be correct any more. That's why my code inserts first and removes later. –  Periodic Maintenance Feb 25 '13 at 17:12
    
@PeriodicMaintenance, good point on the indexes. –  Winston Ewert Feb 25 '13 at 17:32
    
@PeriodicMaintenance, I can see why you would have insert change the order and append keep it. But it doesn't follow the way those methods work on lists. That's why I think it'll be surprising and confusing to the user of this class.Basically, if append doesn't actually append, you shouldn't call it append. –  Winston Ewert Feb 25 '13 at 17:41
    
@PeriodicMaintenance, my suggestion is to make an OrderedSet rather than a unique list. Follow the set interface rather then the list interface. See the OrderedDict class in the collections module for a related example. As it stands you have the same methods as a list, but don't act like a list. But your class would act like a set, just with guaranteed orders and additonal methods. –  Winston Ewert Feb 25 '13 at 17:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.