Let’s try a different approach this afternoon. I have a good feeling about this one. Things go fairly well, but not fantastically well.

Of course I wouldn’t have tried the preceding ideas had I not had a good feeling. Still, this seems much more reasonable.

I was going to review all my plans and dreams and theories. Instead, foolishly I suppose, I’m going to just work on this idea:

Modify the code so that CellBank is the persistent container of all the Cells, so that when a Cell is used in a Room or elsewhere, it’s not removed from the bank, but instead records its status internally. Meanwhile, Cell instances will know the bank, because we want to use the actual cells from the bank, not duplicates as we have been. I propose to make these changes by refactoring, breaking as little as possible.

The biggest risk here is that these changes will turn out to be inadequate and not a step along the way to adequate, because I haven’t really reviewed the needs and the fit of this idea: instead, I’m going with my gut feelings about what we need.

All tests are green except for this ignored one:

    @pytest.mark.skip("not ready")
    def test_edge_neighbors(self):
        space = CellSpace(2,3 )
        cell = space.at(0,0)
        neighbors = cell.neighbors
        assert len(neighbors) == 2

There is no longer a CellSpace class, among other issues. We’ll fix that up when it’s time.

So the plan is to make sure all the Cells have the CellBank, to change things so that the bank is basically immutable after initialization, and the available status of a cell is part of the Cell’s knowledge, not the bank’s.

I think the cells are receiving the bank:

class CellBank:
    def __init__(self, max_x, max_y):
        self.cells = set()
        for x in range(max_x):
            for y in range(max_y):
                self.cells.add(Cell(x, y, self))

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

I think we’ll leave the name space and possibly rename CellBank somewhere along the way, if this works out.

Now how do we determine availability?

    def has_cell(self, x, y):
        return Cell(x, y, None) in self

    def take_xy(self, x, y):
        self.take(Cell(x, y, None))

    def take(self, cell: Cell):
        self.cells.discard(cell)

We want has_cell to fetch the cell and ask it if it is available. So let’s add a test about that:

    def test_available(self):
        space = CellBank(5, 5)
        assert space.has_cell(3,3)
        space.take_xy(3, 3)
        assert not space.has_cell(3,3)

That’s green. Let’s commit it: test for cell available.

Now we’ll need to decide how a Cell will know it’s available. Let’s give it another member variable owner, and init that to the space, and clear it on take. We’ll actually want to change things a bit, changing take to some kind of move_to operation, but small steps.

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

    def is_available(self):
        return self.owner == self.space

Now we can use that to answer the question in CellBank (but still have to deal with how we take it). Ah, an issue: CellBank is a set of cells and isn’t addressable by x and y. Let’s make it have an array of cells as we had with CellSpace.

class CellBank:
    def __init__(self, max_x, max_y):
        self.cells = set()
        self.cell_array = []
        for x in range(max_x):
            col = []
            self.cell_array.append(col)
            for y in range(max_y):
                cell = Cell(x, y, self)
                self.cells.add(cell)
                col.append(cell)

    def has_cell(self, x, y):
        cell = self.cell_array[x][y]
        return cell.is_available()

    def take_xy(self, x, y):
        cell = self.cell_array[x][y]
        cell.take()

    def take(self, cell: Cell):
        self.take_xy(cell.x, cell.y)

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

    def is_available(self):
        return self.owner == self.space

    def take(self):
        self.owner = None

I really felt this was OK but three tests break. Here’s one:

    def test_build(self):
        random.seed(234)
        bank = CellBank(64, 56)
        size = 100
        origin = Cell(32, 28, None)
        room = Room(bank, size, origin)
        assert len(set(room.cells)) == 100
        assert room.origin == Cell(32, 28, None)
        self.verify_contiguous(room)

It’s only getting 28 cells in the room. Let’s find a simpler test, the issue is probably small.

    def test_flood_uses_bank(self):
        bank = CellBank(5, 5)
        taken = [(3,3), (1,4), (2,1)]
        for t in taken:
            bank.take_xy(*t)
        finder = PathFinder(bank)
        finder.put(Cell(1, 2, None))
        finder.flood(None)
        assert len(finder.reached) == 25 - len(taken)
        for t in taken:
            assert t not in finder.reached

Not actually simpler but it’s clear that we’re not removing cells from the set part of CellBank, and we should do that.

    def take_xy(self, x, y):
        cell = self.cell_array[x][y]
        self.cells.discard(cell) # <===
        cell.take()

All tests are now passing, except for the skipped one of course. So that was the issue.

But we really want to get rid of that set in CellBank, so let’s see what’s going on with it. Pathfinder must be asking the wrong question or something, for the test above to fail.

There’s yer problem:

    def can_use(self, neighbor) -> bool:
        if neighbor in self.bank:
            return True
        for room in self.room_list:
            if neighbor in room:
                return True
        return False

Wait, though, we are green. Commit: refactoring CellBank and Cell.

Tried this:

class PathFinder:
    def can_use(self, neighbor) -> bool:
        if neighbor.is_available():
            return True
        for room in self.room_list:
            if neighbor in room:
                return True
        return False

We loop indefinitely. After a bit of head-bashing, I discover that we have cells not in the bank set that are not marked as unavailable. Ah, I bet we’re creating cells with no space defined.

That won’t do. Let’s see if it is happening. Yes. I make this change:

class Cell:
    def __init__(self, x, y, space):
        self.x = x
        self.y = y
        if space is None:
            space = CellBank(0,0) # hack
        self.space = space
        self.owner = space

Test collect failing, recursive import. Back out the hack. Let’s fix the signature of Cell to require the space. It’ll just take a moment with any luck.

    @property
    def neighbors(self):
        deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
        neighbors = []
        for dx, dy in deltas:
            new_x = self.x + dx
            new_y = self.y + dy
            neighbors.append(self.space.at(new_x, new_y))
        return neighbors

Fixing neighbors along the way. One test working now. Down to nine broken. Now eight:

    def test_neighbors(self):
        bank = CellBank(5, 5)
        cell = bank.at(2,3)
        neighbors = cell.neighbors
        assert len(neighbors) == 4
        for n in [bank.at(1, 3), bank.at(3, 3), bank.at(2, 2), bank.at(2, 4)]:
            assert n in neighbors

What we had to change here was the open creation of raw cells. We need always to get them from the bank now. This is a good change, I think, but if the system was large it would be a difficult one.

Unless … could I hack the constructor so that, given a bank provided, I return the existing cell, not a newly created one? I don’t think so, at least not by modifying __init__. I think you can go in and mess with __new__ or something but that’s too deep in the bag.

I’ll just fix all the creators of Cell, shouldn’t take long. Time me. 1456 … 1502, six minutes. Down to four failing. Changed Cell(x, y, z) to bank.at(x, y) throughout. Still issues with PathFinder though.

    def test_flood(self):
        bank = CellBank(5, 5)
        finder = PathFinder(bank)
        finder.put(bank.at(1, 2))
        finder.flood(None)
        assert len(finder.queue) == 0
        assert len(finder.reached) == 25

This is failing during flood:, in expand in neighbors.

class CellBank:
    def at(self, x, y):
>       return self.cell_array[x][y]
               ^^^^^^^^^^^^^^^^^^
E       IndexError: list index out of range

OK:

class Cell:
    @property
    def neighbors(self):
        deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
        neighbors = []
        for dx, dy in deltas:
            new_x = self.x + dx
            new_y = self.y + dy
            neighbor = self.space.at(new_x, new_y)
            if neighbor is not None:
                neighbors.append(neighbor)
        return neighbors

class CellBank:
    def at(self, x, y):
        if 0 <= x < self.width and 0 <= y < self.height:
            return self.cell_array[x][y]
        else:
            return None

We are green. Commit: refactoring Cell and CellBank. CellBank set remains to be addressed.

I think I’d like to get rid of the __iter__ in CellBank, and the set of cells. There are lots of people calling in. Let’s do __contains__ instead, a better way to support in anyway.

    def __contains__(self, cell: Cell):
        cell = self.at(cell.x, cell.y)
        return cell is not None and cell.is_available()

The meaning of in is a bit naff: it means that the cell exists and is available. We should find those usages and do the right thing.

I’m somewhat fried, however, and the tests are green. Commit: inching toward not having the set.

We still have some uses of cells. but as I am fried, let’s wrap up this session.

Summary

With only a little confusion, and not too much difficulty, we have CellBank acting as the permanent holder of the Cells, and the Cells themselves keeping track of whether they are available.

However I am not entirely pleased:

  1. It was necessary to fix upwards of a dozen creations of Cell and replace them with bank.at calls. In a larger system that might have been infeasible. In some cases, we might decide that the refactoring just couldn’t happen, or we might have resorted to the some deep in the bag singleton kind of creation, which itself would almost certainly have still required changing the callers.

  2. There is still some nastiness in there, with the Cell having lots of dunder methods, including __eq__, __hash__, __iter__, and _repr__. The latter is probably OK. The others seem questionable at best. I think __iter__ is used to return the x and y from a cell, which shouldn’t be all that common.

  3. I’m not sure that the notion of having an owner that may or may not be the space is a good one. We might do better to have the owner start out None and set it if the cell gets assigned to a room or path.

One good question is whether things would have gone better had I paid more attention to the larger picture of needs. We’ll never know, but it might have led to smaller steps, perhaps starting with different cell details and not creating the new structure.

Truth is, it’s not clear that the arrays are all that much better than the set, except that we had been removing things from the set. One advantage to the arrays is that to index into the set we need a sequence, and we were creating and destroying a lot of them. The array scheme should thrash the garbage collector less. Not something I’d usually worry about.

Overall, I am not displeased … just not entirely pleased. This scheme seems to me to be working and to be going to be better. It’s a little disappointing that it’s tricky to put in, and it’s quite possible that there was a better path to take. But this one has led to a better place, so that’ll do for now.

We’ll hit it again tomorrow!