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.

I wrote a simple class implementing the collections.Sequence interface, that forwards calls to its items to member functions of its items.

I'm requesting a review of the following code snippet.

import collections

class ForwardSequence(collections.Sequence):
    def __init__(self, *args, **kw):
        super(ForwardSequence, self).__init__()
        self._sequence = tuple(*args)
        self._get = kw.pop('get', None)
        self._set = kw.pop('set', None)

    def __len__(self):
        return len(self._sequence)

    def __getitem__(self, item):
        if isinstance(item, slice):
            indices = item.indices(len(self))
            return [self[i] for i in range(*indices)]
        return self._get(self._sequence[item])

    def __setitem__(self, item, value):
        items = self._sequence[item]
        if not isinstance(items, collections.Sequence):
            items = (items,)
        for i in items:
            self._set(i, value)

E.g.

class Command(object):
    def query(self):
        # send and return query string.

    def write(self, value):
        #send write string.

class Controller(object):
    '''A simple temperature controller.'''
    def __init__(self):
        self.temperature = ForwardSequence(
            (Command() for _ in range(10)),
            get=lambda x: x.query(),
            set=lambda x, v: x.write(v)
        )

ctrl = Controller()

# calls the query method of the first 5 Commands and returns it's results
print ctrl.temperature[0:5]
share|improve this question

1 Answer 1

up vote 1 down vote accepted
import collections

class ForwardSequence(collections.Sequence):
    def __init__(self, *args, **kw):
        super(ForwardSequence, self).__init__()
        self._sequence = tuple(*args)

A tuple only takes on argument, why are you passing a variable number of arguments?

        self._get = kw.pop('get', None)
        self._set = kw.pop('set', None)

Why don't you use keyword arguments instead of **kw? As it stands you are asking for trouble by ignoring any arguments that aren't there. Also, do you really want to allow the user to avoid specifying get and set. It seems to me those will always need to be defined.

    def __len__(self):
        return len(self._sequence)

    def __getitem__(self, item):
        if isinstance(item, slice):
            indices = item.indices(len(self))
            return [self[i] for i in range(*indices)]

I'd suggest

return map(self._get, self_sequence[item])

That should offload some of the work onto the underlying sequence.

        return self._get(self._sequence[item])

    def __setitem__(self, item, value):
        items = self._sequence[item]
        if not isinstance(items, collections.Sequence):
            items = (items,)

I suggest checking isinstance(item, slice) instead of this. As it stands, it has confusing behaviour of sequence contains sequences. I'd also just call self._set(item, value), as you aren't saving anything by storing the item in a one-length tuple

        for i in items:
            self._set(i, value)

I'm not sure I like the class's interface. It seems to be doing something more complex while masquerading as a list. This seems problematic to me. But I'd have to see more about how you use to know for certain.

share|improve this answer

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.