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'm requesting a review of the following code snippet.

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

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]

Why should I wanna do this?

I'm writing interfaces to measurement equipment. Therefore I have to generate, send and receive commands. Having to write e.g.

ctrl.voltage = 12.8 # Generates 'VOLT 12.8' and send this to device.
print ctrl.voltage # Sends 'VOLT?' receives '12.8' and parses this to float

is more intuitive than

ctrl.set_voltage(12.8)
print crtl.get_voltage()

This can easily be implemented via properties. Now let's assume I've got 10 temperature sensors I'd like to read. To print them I could write

print ctrl.temperature1
...
print ctrl.temperature10

But this looks horrible. It would be more elegant if I could iterate them as if it were a sequence.

for temp in ctrl.temperatures:
    print temp

Update

import collections

class ForwardSequence(collections.Sequence):
    def __init__(self, iterable, get, set=None):
        super(ForwardSequence, self).__init__()
        self._sequence = tuple(iterable)
        self._get = get
        self._set = set

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

    def __getitem__(self, item):
        if isinstance(item, slice):
            return map(self._get, self._sequence[item])
        return self._get(self._sequence[item])

    def __setitem__(self, item, value):
        if not self._set:
            raise RuntimeError('Item not settable')
        if isinstance(item, slice):
            for i in self._sequence[item]:
                self._set(i, value)
        return self._set(item, value)
share|improve this question
    
This code won't work, where is get coming from in the constructor? Did you test this? –  Lattyware Nov 9 '12 at 13:22
    
@Lattyware Ups fixed it. –  P3trus Nov 9 '12 at 13:26
add comment

1 Answer

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
    
I updated the code with your suggestions. –  P3trus Nov 12 '12 at 15:32
add comment

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.