Designing

Hello, loves!

This afternoon I want to explore some alternative notions around the Cell topics raised this morning. Very promising results!

Here, as a refresher, are this morning’s thoughts:

There are a couple of issues that come up for me, looking back over the morning:

1. There are a lot of methods in DungeonLayout about cell is this cell is that. This is feature envy: Cells are not very helpful. They have essentially no behavior and do not know anything beyond their coordinates. I do not see a way to improve that much, unless we gave them all a pointer back to the layout. And that seems wrong to me.

2. Similarly, if a Cell knew its Room that would sometimes be helpful. Again, we do not like to have objects pointing to their containers.

3. A low level design property of the current code is that methods returning a Cell can also return None, which means that a lot of other code then has to check that result. It is possible that a Missing Object approach might be helpful here. That might be an educational thing to work on.

This issue has kind of been a low-level ache right along. I would like to improve things. Ideas include:

• Make the cells into a four-way linked structure where every cell knows up to four neighbors.
• Perhaps there could be two kinds of links, direct and passage, with logic in them.
• Perhaps only cells that are in rooms actually exist. Cell available = cell not in any room
• Content is a dictionary in Dungeon from cell to a contents list. Maybe let the cell have its contents?

One seemingly useful idea would be for a Cell to know its room. As things stand, however, we often create cells with a call to Cell, because they have no behavior outside some coordinate manipulation:

class Cell:
    def __init__(self, x, y):
        self._xy = (x,y)

    def __eq__(self, other):
        return self.xy == other.xy
    def __hash__(self):
        return hash(self.xy)
    def __iter__(self):
        return iter(self.xy)
    def __repr__(self):
        return f"Cell({self.x}, {self.y})"

    @property
    def x(self):
        return self._xy[0]

    @property
    def y(self):
        return self._xy[1]

    @property
    def xy(self):
        return self._xy

    def center_position(self, size):
        cx = self.x*size + size // 2
        cy = self.y*size + size // 2
        return cx, cy

    def vector_diff(self, target):
        return self.x - target.x, self.y - target.y

    def manhattan_distance(self, target):
        return abs(self.x - target.x) + abs(self.y - target.y)

    def distance(self, target):
        return math.hypot(*self.vector_diff(target))

If we’re going to store things in the Cell, we need always to return the same Cell instance. One way to do that might be to override __new__, perhaps using a class variable to refer to a DungeonLayout, or perhaps some less complicated layout-like object.

I think I’ll write a couple of tests to see what might work.

    def test_cell_is_cell(self):
        layout = DungeonLayout(10, 10)
        c_00 = Cell(0,0)
        l_00 = layout.at(0,0)
        assert l_00 is c_00

This test fails, because c_00 is a new cell, and l_00 is one from the layout, and they are not identical.

Let’s see if we can make that work.

class Cell:
    layout = None

    def __new__(cls, x, y):
        if cls.layout:
            return cls.layout.at(x,y)
        else:
            cell = super().__new__(cls)
            cell._xy = (x, y)
            return cell


class DungeonLayout:
    def __init__(self, max_x=10, max_y=10):
        self.max_x = max_x
        self.max_y = max_y
        self.border_map = None
        self.cells = dict()
        Cell.layout = None
        for x in range(max_x):
            for y in range(max_y):
                self.cells[(x,y)] = Cell(x, y)
        Cell.layout = self
        self.rooms = []
        self.passages = dict()
        self.room_map = dict()

In Layout, we have to set the Cell layout to None, so that we get the normal creation, and then we set it to self so that henceforth we get only existing cells.

All the tests go green. I think that’s nearly OK but I think we can use the old init:

class Cell:
    layout = None

    def __new__(cls, x, y):
        if cls.layout:
            return cls.layout.at(x,y)
        else:
            return super().__new__(cls)

    def __init__(self, x, y):
        self._xy = (x,y)

That continues to work. Let’s not commit this, we’ll treat it as a spike for now. I have a concern about the way we have to set the layout to None, build the layout’s cells, and then set the layout to self. We could forget that.

It would be nice to be able to write this:

class DungeonLayout:
    def __init__(self, max_x=10, max_y=10):
        self.max_x = max_x
        self.max_y = max_y
        self.border_map = None
        self.cells = dict()
        with Cell.creating(self):
            Cell.layout = None
            for x in range(max_x):
                for y in range(max_y):
                    self.cells[(x,y)] = Cell(x, y)
        self.rooms = []
        self.passages = dict()
        self.room_map = dict()

And we can say this:

class Cell:
    layout = None

    @classmethod
    @contextmanager
    def creating(cls, layout):
        Cell.layout = None
        try:
            yield
        finally:
            Cell.layout = layout

    def __new__(cls, x, y):
        if cls.layout:
            return cls.layout.at(x,y)
        else:
            return super().__new__(cls)

    def __init__(self, x, y):
        self._xy = (x,y)

The @contextmethod runs whatever is inside the with, first clearing the layout, then running the block, then setting the layout, all inside. So all we have to know about this is inside Cell, and DungeonLayout, with the latter only knowing that it has to use Cell.creating to get the job done.

I think this is good, and all of our tests are running. There is, in essence, no difference, except that now, in normal operation, if a layout is active, Cell(x,y) will refer to the instance in the layout. This means that we can, in principle, remove all use of layout.at except the one in Cell. If I’m not mistaken.

Let’s commit this: new Cell instances are put into a Layout if one is active.

What happens now if we try to access a cell that doesn’t exist, i.e. is outside the layout?

# cell access
    def at(self, x, y):
        return self.at_xy((x,y))

    def at_xy(self, xy):
        return self.cells.get(xy, None)

    def at_offset(self, start_xy, offset_xy):
        new = start_xy[0] + offset_xy[0], start_xy[1] + offset_xy[1]
        return self.at_xy(new)

Pretty sure we get none. Let’s write a test if we don’t have one already.

    def test_out_of_range(self):
        layout = DungeonLayout(1, 1)
        cell = Cell(3,3)
        assert cell is None
        cell = Cell(-1,-1)
        assert cell is None
        cell = Cell(0,0)
        assert cell is not None
        offset = cell.offset_by(1,1)
        assert offset is None

class Cell:
    def offset_by(self, x, y):
        return Cell(x+self.x, y+self.y)

Added that new method.

There is this method, used four times:

class DungeonLayout:
    def at_offset(self, start_xy, offset_xy):
        new = start_xy[0] + offset_xy[0], start_xy[1] + offset_xy[1]
        return self.at_xy(new)

I replace those calls with calls to a new version of offset_by:

class Cell:
    def offset_by(self, offset):
        x,y = offset
        return Cell(x+self.x, y+self.y)

We are green, without the at_offset method, referring the question to Cell, where it rightly belongs, or so it seems to me.

Commit: moving at_offset users to use Cell.offset_by.

All is well and the tests are no slower than they were. There’s at most one method dispatch difference between old and new, so it would be surprising if we could see any effect.

Hm. Let’s sum up.

Summary

OK, first of all, it is [DELETED] hot out there. I can see why people are dropping dead, I nearly died dragging the bit to the curb. A mere 35C. I can remember when I was a mere youth, enjoying weather like this. Not today. But I digress.

By providing and setting the layout class variable in Cell, we have given all Cell instances access to their relatives at any offset from the source cell, with a None return (so far) when an out-of-range cell is accessed. The change has no noticeable effect on performance, but it does involve some deep-in-the-bag capabilities:

1. We override __new__ in Cell, which is rare.
2. We provide a Context Manager in Cell, to ensure that the layout variable is properly toggled in and out. I’m pretty confident that this was my first implementation of a Context Manager.

What we buy with this change, so far is that we have already simplified over 100 lines, mostly in tests. And I have high hopes and aspirations for the idea. Things to explore include:

Providing access to the Room that the Cell is in should be easier, using a property on Cell that defers the question to the layout, or, optionally, by storing the room directly in the Cell. That will simplify code like this:

class DungeonLayout:
    def are_in_same_room(self, cell, new_cell):
        return cell and new_cell and self.get_room(cell) == self.get_room(new_cell)

To this.

class DungeonLayout:
    def are_in_same_room(self, cell, new_cell):
        return cell and new_cell and cell.room == new_cell.room 

class Cell:
    @property
    def room(self):
        return self.layout.get_room(self)

That works, I just typed it in. Commit: Cell knows room.

However, that’s just the beginning. There are all kinds of method in layout where we had to sail around the horn to get things done, like this:

class DungeonLayout:
    def is_in_a_room(self, cell) -> bool:
        return cell and self.get_room(cell) is not None

We can improve this slightly now:

    def is_in_a_room(self, cell) -> bool:
        return cell and cell.room is not None

This is more clear, even though it does bounce through Cell and back into the layout. I prefer clear.

But if we had a MissingCell object instead of None, we could probably do even better.

It’s early days, but I have the feeling that this little change will make the code simpler to write and easier to understand.

I am pleased, so far. Now, if the power and internet ever return, I’ll publish this. See you next time, I hope!