Ring datatype in Python

I wanted to write something that would be useful for the indices of a grid where if one walked off the edge of the grid they would re enter on the opposite side. This could in theory help with checking for special cases when checking for neighbours to a certain position on the grid. For example the neighbours of position (0, 0) on a 10 x 10 grid would be (0, 1), (1, 1), (1, 0), (9, 9), (0, 9), (1, 9), (9, 0), (9, 1).

def _adj_index(method):
    def _dec(instance, *args):
        if isinstance(args[0], int):
            index = args[0] % len(instance)
            return method(instance, index, *args[1:])
        return method(instance, args)
    return _dec

class Ring(list):
    """
    Ring datatype.
    """

    def __iter__(self):
        while True:
            for i in xrange(len(self)):
                yield self[i]

    def __getitem__(self, item):
        if isinstance(item, tuple):
            slice = item[0]
            return Ring([
                self[i]
                for i in xrange(slice.start, slice.stop, slice.step)])
        return super(Ring, self).__getitem__(item)

    __getitem__ = _adj_index(__getitem__)
    __setitem__ = _adj_index(list.__setitem__)
    __delitem__ = _adj_index(list.__delitem__)

    def __getslice__(self, i, j):
        return Ring([
            self[x]
            for x in range(i, j)])

I don't like the special case for __getitem__ of having to check if it is an integer or a tuple containing a slice.

You shouldn't inherit from builtin data types - there's a UserList type that's more appropriate.

On top of that, you probably shouldn't inherit from UserList either: your code is not a list and doesn't fill its contractual obligations. This is a time to favour composition over inheritance.

Before I look at implementing this, here are a few quick things I noticed. One is

if isinstance(item, tuple):
    ...

in __getitem__, which doesn't seem to work since _adj_index is packing any tuples it gets into another tuple.

Another thing was the use of for i in xrange(len(self)):, which would be much simpler if you could just do for i in self._items - another advantage of composition. You can actually manage this with super(Ring, self).__iter__(), though.

Your __getslice__ returns another ring; this seems strange - a slice of a ring is rarely itself also a ring. If you have a ring of paper and cut a section out, that section has ends!

Finally, your __str__ and __repr__ are the default ones for list, which is entirely misleading and very bad practice.

Back to the point, looking at the abstract base classes in collections (later moved to collections.abc), the only interfaces it makes sense to support are Container and Iterable, since the class doesn't have a __len__. Adding __{get,set,del}item__ and __repr__ on top of that would give something like

def _adj_index(method):
    def _dec(instance, *args):
        if isinstance(args[0], int):
            index = args[0] % len(instance._items)
            return method(instance._items, index, *args[1:])
        return method(instance._items, args)
    return _dec

class Ring:
    """
    Ring datatype.
    """

    def __init__(self, items):
        self._items = list(items)

    def __iter__(self):
        while True:
            for i in xrange(len(self._items)):
                yield self._items[i]

    __getitem__ = _adj_index(list.__getitem__)
    __setitem__ = _adj_index(list.__setitem__)
    __delitem__ = _adj_index(list.__delitem__)

    def __getslice__(self, i, j):
        return [self[x] for x in range(i, j)]

    def __repr__(self):
        return "Ring({})".format(self._items)

This discards __getitem__ since it's no longer needed but keeps it similar to your code.

__iter__ could just delegate to itertools.cycle.

__getslice__ is deprecated and has been for ages. Let's consider doing this properly:

def __getitem__(self, idx):
    if isinstance(idx, slice):
        return ???

    return self._items[idx % len(self._items)]

What should ??? be? Well, we'd like something akin to

[self[i] for i in xrange(idx.start, idx.stop, idx.step)]

but start, stop or step might be None. We can say that start and stop should not be None (since both directions extend infinitely), but step can reasonably be asked to default to 1. Unfortunately, slice.indices isn't appropriate here, as with other tricks you can normally use.

Thus:

step = 1 if idx.step is None else idx.step
return [self[i] for i in xrange(idx.start, idx.stop, step)]

Now, __setitem__ probably has no good semantics for slice assignment because:

-
If a slice is larger than len(self._items), there is no obvious cannonical mapping on how to handle the assignment.

-
If a slice crosses through the start index (has idx % len(self._items) == 0) and assigns to a slice of a different size, there is no obvious way to decide where the new start is moved to.

You could restrict slice assignments to slices of the same size, and on slices no more than len(self._items) in size - but then you're not giving much bang for the buck. For this reason, I think __setitem__ and __delitem__ should remain as they are, operating only on integers.

However, _adj_index is not worth its cost. You could just implement them as

def __setitem__(self, idx, value):
    if not isinstance(idx, int):
        raise TypeError("Ring indices must be integers, not {}".format(type(idx)))

    self._items[idx % len(self._items)] = value

def __delitem__(self, idx):
    if not isinstance(idx, int):
        raise TypeError("Ring indices must be integers, not {}".format(type(idx)))

    del self._items[idx % len(self._items)]

(the error mirrors that of indexing lists).

In the scheme of things, this code is simpler, faster and contains not too much logical duplication. However, deduplication is nice, so one could merely add a _from_integer_index method:

```
def _from_integer_index(self, idx):
if not isinstance(idx, int):
raise TypeError("Ring indices must be integers, not {}".format(type(idx)))

return idx % len(self._items)

def __getitem__(self, idx):
if isinstance(idx, slice):
step = 1 if idx.step is None else idx.step
return [self[i] for i in xrange(idx.start, idx.stop, step)]

return self._items[self._from_integer_index(idx)]

def __setitem__(self, idx, value):
self._i

StackExchange Code Review Q#85050, answer score: 2