Small Stuff
With a little help from my friends, I set out to smooth some rough edges. Lots of small things. Nothing to see here, aside from one excellent name change. A metaphor issue discussed at the end.
Last night, Tuesday was Friday Geeks Night Out, a weekly Zoom session with some of my friends. We talk about many things, but many of our best sessions are the ones where we look at people’s code. Last night we looked at the dungeon code again, since the previous week’s input had been rather thin for some reason. This week we were a bit more alert, and turned up a few things. We’ll look at at least one of those here, and then see where we are led by what we find.
Periphery?
In the Room class, there is a member variable called periphery. Here’s the code that manipulates it:
class Room:
def _build(self, space, number_of_cells):
new_cell: Cell = self.origin
for _ in range(number_of_cells):
self.take(new_cell)
if new_cell.is_periphery:
self.periphery.append(new_cell)
new_cell = self.find_adjacent_cell(space)
if new_cell is None:
return
def find_adjacent_cell(self, bank: CellSpace):
for cell in sample(self.periphery, len(self.periphery)):
self.probe_count += 1
neighbors = cell.neighbors
for neighbor in sample(neighbors, len(neighbors)):
if neighbor.is_available():
return neighbor
self.periphery.remove(cell)
return None
The reason for that periphery stuff is efficiency. A while back, I noticed that the dungeon building took discernible time, a second or two. So I set out to improve it. Sort of thing one does. One issue was that as a room grows, more and more of its cells are interior to it, and those cells cannot be used for expansion. Only cells that are what I thought of as “on the periphery” can extend the room. These are the cells that still have at least one neighbor that is unassigned.
In the find_adjacent_cell above, we select a random cell from the periphery, and check to see if it has any available neighbors. If it does, we return it to be used. If it does not have any available neighbors, it is no longer on the periphery and we remove it.
In _build, the code used to just assume that the added cell probably had an available neighbor, and added it to the periphery. If perchance it has none, and if perchance it shows up in a later expansion attempt, we’ll detect that it has no neighbors and remove it.
The FGNO team felt that it wasn’t proper to add it to the periphery unless we knew it belonged there. That’s why I added the is_periphery property on Cell:
@property
def is_periphery(self):
for n in self.neighbors:
if n.is_available():
return True
return False
def is_available(self):
return self.owner == self.space
- Note
- In the code above we see inconsistency between which aspects of Cell are properties and which are methods. That’s on the list to be sorted out. The FGNO didn’t notice that, but I did.
We had some discussion of whether the word periphery was causing the objection, but we didn’t come up with any alternatives that were helpful. However, in writing the above, I have come up with a name that I think resolves the issue favorably.
- Note
- I don’t like having that check in
_build, because it occurs unconditionally, while the check infind_adjacent_celloccurs only if the “periphery” cell is later chosen for growth. The code may arguably be more clear as it is now, but it is also less efficient. -
But here comes the fix.
Let’s rename periphery!
def _build(self, space, number_of_cells):
new_cell: Cell = self.origin
for _ in range(number_of_cells):
self.take(new_cell)
self.growth_candidates.append(new_cell)
new_cell = self.find_adjacent_cell(space)
if new_cell is None:
return
def find_adjacent_cell(self, bank: CellSpace):
for cell in sample(self.growth_candidates, len(self.growth_candidates)):
self.probe_count += 1
neighbors = cell.neighbors
for neighbor in sample(neighbors, len(neighbors)):
if neighbor.is_available():
return neighbor
self.growth_candidates.remove(cell)
return None
Excellent name change!
We call them “growth candidates”! They’re not guaranteed to be suitable, they are just candidates. Later on, if their number comes up, we either use them or discover that they are no longer candidates and remove them from the list. I think this resolves the issue that the FGNO raised, and it certainly meets my criteria, allowing me to get rid of that duplicate and redundant as well as repetitive check.
In starting to commit, I found a line I’d like to change:
class RoomView:
def draw(self, screen):
cells = set(self.room.cells)
periphery = self.room.growth_candidates
for cell in cells:
self.draw_colored_cell(cell, screen)
self.draw_border(cell, cells, screen)
That’s clearly wasted, must be left over from something I changed. Delete the line, test, commit: growth_candidates.
Now over to Cell, since we no longer need is_periphery. Removed. Commit: remove unused property.
There is only one other property in Cell and it is frankly weird:
class Cell:
@property
def neighbors(self):
return [neighbor
for dxy in self.deltas
if (neighbor := self.at_offset(*dxy))]
Are the neighboring cells of a cell a property, or are they computed? They are in fact computed but the result is always the same. I think this can remain a property. That being the case, there are some other methods that could be properties as well:
class Cell:
def is_available(self):
return self.owner == self.space
def is_in_a_room(self):
return not self.is_available()
I think I’d like to make those be properties as well. I’ll insert some brief discussion in a moment.
- Properties vs Methods
- I freely grant that I am inconsistent when it comes to properties vs methods. My reflexes when just coding away will typically have me writing methods only, but when I’m a bit more thinky, I like things that could be named
is_somethingto be properties. I’m trying to become more consistent, but so far I haven’t come up with a firm set of rules that I like. -
Expect further inconsistency. Whether I’ll be eventually consistent on this, I do not know.
Anyway, making both of those be properties goes smoothly between PyCharm’s ability to find references, and my tests. Commit: make is_ methods properties.
The search for places to change led me to CellSpace, where we have two issues. One is that the file is still named “cell_bank.py”. Rename it. Commit, amazed at all the files that changed, basically everyone who imports CellSpace.
Ah. Now that we’re looking at CellSpace, I’m reminded of another concern from FGNO.
class Room:
def _build(self, space, number_of_cells):
new_cell: Cell = self.origin
for _ in range(number_of_cells):
self.take(new_cell)
self.growth_candidates.append(new_cell)
new_cell = self.find_adjacent_cell(space)
if new_cell is None:
return
def take(self, new_cell: Cell):
self.cells.append(new_cell)
new_cell.owner = self
There is fuzziness here, in the notions of taking and owning, and being in a room. In CellSpace, this fuzziness becomes more visible:
class CellSpace:
def at(self, x, y) -> Cell|None:
if 0 <= x < self.width and 0 <= y < self.height:
return self.cell_array[x][y]
else:
return None
def has_cell(self, x, y):
# should check range?
cell = self.cell_array[x][y]
return cell.is_available
def take_xy(self, x, y, owner):
# should check range?
cell = self.cell_array[x][y]
cell.take(owner)
def take(self, cell: Cell, owner):
self.take_xy(cell.x, cell.y, owner)
Let’s try to think clearly about what CellSpace really is and does. It holds a 2D array of all the cells indexed by x and y, so that the object appears to be a 2D array of cells at x,y.
Cells are never removed from CellSpace, nor are they added or replaced. It’s just a static array of all the cells we have. So the notion of taking a cell is flawed. Tests aside, if there are any doing it, if we have a cell, we might want to put it into a room, as we see above in new_cell.owner=self, which assigns the owner field to be the room.
Why owner? Why not room? History. I thought there might be other kinds of owners. Premature generalization. Let’s see about renaming owner to room.
Just renaming it breaks three tests. I’ll have a quick look and if it’s not clear what happened, roll back.
Just a matter of finding calls to room(). There were three.
Along the way I discovered this in Cell:
def border_at(self, x, y):
owner = self.room
neighbor = self.at_offset(x, y)
if neighbor is None:
return "wall"
elif neighbor.room == self.space:
return "wall"
elif neighbor.room == owner:
return "open"
else:
return "door"
That assignment isn’t helping. Inline:
def border_at(self, x, y):
neighbor = self.at_offset(x, y)
if neighbor is None:
return "wall"
elif neighbor.room == self.space:
return "wall"
elif neighbor.room == self.room:
return "open"
else:
return "door"
Forgot to commit: changing owner to room.
Now about this:
class Cell:
deltas = [(-1, 0), (1, 0), (0, -1), (0, 1)]
def __init__(self, x, y, space):
self.x = x
self.y = y
assert space is not None
self.space = space
self.room = space
I think it would make more sense if we were to init room to None. And another thought comes to mind, but first this one. I think it impacts is_available. Sure enough:
@property
def is_available(self) -> bool:
return self.room == self.space
Change that and the init. Twelve (12) tests break. Quick look, else roll back.
Here’s the first one:
def test_available(self):
space = CellSpace(5, 5)
assert space.has_cell(3,3)
space.take_xy(3, 3, None)
assert not space.has_cell(3,3)
Ah. Yes. Historically we used to “take” a cell by telling the space that the cell was taken, providing the object that took it. We don’t want to do that any more. We want to tell the cell that it has a new room (formerly owner). We may need to find another path. I’ll check a few other tests.
Ah! I had made a mistake, fixed here:
class Cell:
@property
def is_available(self) -> bool:
return self.room is None
I had is not None, which is not so good. Tests failing are now down to a more manageable four, but I’m still not sure whether we should roll back.
Here’s the change to the test above:
def test_available(self):
space = CellSpace(5, 5)
assert space.has_cell(3,3)
space.at(3,3).room = 666
assert not space.has_cell(3,3)
Strictly speaking, we should be putting a Room in there, but duck-typing FTW. Test goes green. Next:
def test_border_characterization(self):
space = CellSpace(5, 5)
space.take_xy(3,3, 1)
space.take_xy(2,3, 1)
space.take_xy(4,3, 2)
c_33 = space.at(3,3)
borders = c_33.borders()
assert len(borders) == 4
assert borders["n"] == "wall"
assert borders["e"] == "door"
assert borders["w"] == "open"
assert borders["s"] == "wall"
That turns out to need a change in borders:
def border_at(self, x, y):
neighbor = self.at_offset(x, y)
if neighbor is None:
return "wall"
elif neighbor.room is None:
return "wall"
elif neighbor.room == self.room:
return "open"
else:
return "door"
We need to check neighbor.room for None, used to check space. Down to 2 tests. Still might roll back but I think we might be OK.
def test_cell_bank(self):
bank = CellSpace(10, 10)
assert bank.has_cell(3, 4) is True
bank.take_xy(3, 4, None)
assert bank.has_cell(3, 4) is False
Change the None to anything else and this will pass.
def test_cell_bank(self):
bank = CellSpace(10, 10)
assert bank.has_cell(3, 4) is True
bank.take_xy(3, 4, 666)
assert bank.has_cell(3, 4) is False
Just one more:
def test_constrained_room(self):
random.seed()
bank = CellSpace(10, 10)
for c in range(10):
bank.take_xy(0, c, None)
bank.take_xy(9, c, None)
bank.take_xy(c, 0, None)
bank.take_xy(c, 9, None)
origin = bank.at(5, 5)
size = 100
room = Room(bank, size, origin)
assert len(room.cells) == 64
Change those not to be none and should pass. Yes. Can commit: Cell.owner changed to room, inits None.
We still have these concerning comments:
def has_cell(self, x, y):
# should check range?
cell = self.cell_array[x][y]
return cell.is_available
def take_xy(self, x, y, room):
# should check range?
cell = self.cell_array[x][y]
cell.take(room)
We have this check already:
def at(self, x, y) -> Cell|None:
if 0 <= x < self.width and 0 <= y < self.height:
return self.cell_array[x][y]
else:
return None
Upon due reflection, however, what would we do if we received parameters out of range? Should we silently ignore the issue. Let’s see what use is being made of that None return.
It’s a bit cryptic but here is the main use of the None aspect:
class Cell:
@property
def neighbors(self):
return [neighbor
for dxy in self.deltas
if (neighbor := self.at_offset(*dxy))]
The if will fail if at_offset returns None, so we only collect real neighbors. When we’re at the edges of space, this won’t try to return things from other dimensions.
In the case of the two methods above, I think has_cell should use the None return. But take_xy? If it ever gets out of range, it’s a serious bug and maybe an exception is needed. Let’s be explicit:
def has_cell(self, x, y):
cell = self.at(x, y)
return cell is not None and cell.is_available
def take_xy(self, x, y, room):
cell = self.at(x, y)
if cell is None:
raise IndexError
cell.take(room)
I do not have tests for any of that. My tests do not cover these matters. I’ve made a note on a card to write some.
We are green. Let’s commit and sum up.
Summary
Mostly just small things. The name change to growth_candidates, simple though it was, is something to be proud of. Formerly, the name periphery led to confusion and a demand for a redundant check, in aid of clarity. I think that the FGNO, sticklers though they may be, would be satisfied with that name. And if not, well, they aren’t here now anyway.
The naming of things and deciding whether they are properties is somewhat improved. However, there is still the residual notion of “take” in CellSpace. I used to think of removing the cell from space and putting it into a room. That was never what happened: it remained in space and had a new owner. But the metaphor was in my head. There are tens of calls to the take logic in CellSpace, due to the order in which tests and code were created. I’d like to remove all that and come up with a better metaphor, perhaps “assigning” a cell to a room or something like that, something less like taking it away from anything. I’ve made a note of that.
It commonly happens that at some stage in a design, we realize that the way we have been thinking about things isn’t quite the same as what it used to be: the “metaphor” has changed. Sometimes we can smoothly move over but that often leaves traces of the old notion in the code. They usually won’t trip us up much, but when we see “owner” now, when we’re really using “room”, that’s going to be just a little bump in the road. So I’ve made a note to look for those and change them.
Finally, right now Cell.room is just a member that people access. I’d rather cover that with properties, and we’ll look at that in the future as well.
But for now, our two hours are up, and our line count is way too high, so we’ll call it a morning. Nothing exciting, just a bit of improvement. Every little bit helps, in my view.
See you next time!