Code Review
Kind of at loose ends this morning. Let’s look at the Game of Life code and see if anything comes to mind. Just some small changes, other than reorganizing the files.
The tests run, and my on screen Curses demo runs as well. So that’s good. What’s not so good?
Well, the tests and code are still all in one file. I generally work that way when creating a something new, even a new class in an existing program, until the file gets too big to allow for quick bouncing from test to code. This may be a bad habit. Anyway, let’s move the Grid class to its own file in the ‘src’ folder.
PyCharm does that automatically, of course: I just have to type in the folder and file name. It did leave the type definitions in the test file. Let’s see if we can move those automatically. We cannot: that’s probably why it didn’t offer that possibility. Cut and paste them. Tests still run: test file didn’t use them.
After more hassle than I’d like to admit, I have four separate files, main, grid, curses_view and test_conway. We’re green and the Terminal example works. Commit. Here’s the grid file I’m looking at:
from collections import defaultdict
from typing import Self
type GridCoordinates = tuple[int, int]
type LifeGrid = set[GridCoordinates]
type NeighborDict = defaultdict[GridCoordinates, int]
class Grid:
def __init__(self, cells=None) :
self.cells: LifeGrid = cells if cells else set()
def __len__(self):
return len(self.cells)
def add(self, x, y) -> None:
self.cells.add((x, y))
def evolve(self) -> Self:
cells = self.next_generation()
return self.__class__(cells)
def next_generation(self) -> LifeGrid:
neighbor_counts: NeighborDict = self.count_neighbors()
stay_alive = self.get_staying(neighbor_counts)
born = self.get_born(neighbor_counts)
return stay_alive | born
def get_born(self, neighbor_counts) -> LifeGrid:
was_already_alive = self.cells
threes: LifeGrid = {cell for cell, num in neighbor_counts.items() if num == 3}
born = threes - was_already_alive
return born
def get_staying(self, neighbor_counts) -> LifeGrid:
was_already_alive = self.cells
twos_and_threes: LifeGrid = {cell for cell, num in neighbor_counts.items()
if num in {2, 3}}
return twos_and_threes & was_already_alive
def count_neighbors(self) -> NeighborDict:
neighbors = [(-1, -1), (-1, 0), (-1, 1), (0, -1), (0, 1), (1, -1), (1, 0), (1, 1)]
neighbor_counts: NeighborDict = defaultdict(int)
for x, y in self.cells:
for dx, dy in neighbors:
neighbor_counts[(dx + x, dy + y)] += 1
return neighbor_counts
def has(self, pair) -> bool:
return pair in self.cells
@property
def size(self) -> int:
return len(self.cells)
def as_string(self, x0=0, y0=0, x1=10, y1=10) -> str:
alive = '*'
dead = '.'
lines = []
for y in range(y0, y1):
row = [alive if (x,y) in self.cells else dead for x in range(x0, x1)]
lines.append(''.join(row))
return '\n'.join(lines)
I notice that we are supporting len with the __len__ member, and we also have size as a property. Relatedly there is has, which is a lot like Python’s in. Let’s enhance a test to test len and in and then make them work and make the other methods go away.
def test_blinker_oo(self):
grid = Grid()
grid.add(5, 4)
grid.add(5, 5)
grid.add(5, 6)
grid = grid.evolve()
assert grid.size == 3
assert len(grid) == 3
assert grid.has((4,5))
assert (4,5) in grid
assert grid.has((5,5))
assert grid.has((6,5))
len should work already. and I expect that in will tell me what I need to do. Test.
> assert (4,5) in grid
^^^^^^^^^^^^^
E TypeError: argument of type 'Grid' is not iterable
Right. Needs to be iterable. PyCharm figures out what I’m doing as soon as i type def __iter__ and we get:
def __iter__(self) -> Iterator[GridCoordinates]:
return iter(self.cells)
I would not have known how to do the type hint, but I actually did know the rest. Really.
Green. Remove the size and has methods and their checks, replacing with len and in as appropriate.
Did size first. Only one usage anyway. Commit. has had three. Replaced. Green. Commit.
I don’t see much else. The name get_born sounds more like a command than a computation of a collection of newborn cells, and get_staying is also odd. Let’s review the use of those two methods:
def next_generation(self) -> LifeGrid:
neighbor_counts: NeighborDict = self.count_neighbors()
stay_alive = self.get_staying(neighbor_counts)
born = self.get_born(neighbor_counts)
return stay_alive | born
Let’s rename the temps. I think we’ll keep them, though we could inline, but the return line would be messy if we did.
def next_generation(self) -> LifeGrid:
neighbor_counts: NeighborDict = self.count_neighbors()
cells_staying_alive = self.get_staying(neighbor_counts)
cells_newly_born = self.get_born(neighbor_counts)
return cells_staying_alive | cells_newly_born
I like that. Green. Commit. Let’s rename the methods similarly. I reordered the call to match the order of the methods below.
def next_generation(self) -> LifeGrid:
neighbor_counts: NeighborDict = self.count_neighbors()
cells_newly_born = self.get_cells_newly_born(neighbor_counts)
cells_staying_alive = self.get_cells_staying_alive(neighbor_counts)
return cells_newly_born | cells_staying_alive
def get_cells_newly_born(self, neighbor_counts) -> LifeGrid:
was_already_alive = self.cells
threes: LifeGrid = {cell for cell, num in neighbor_counts.items() if num == 3}
born = threes - was_already_alive
return born
def get_cells_staying_alive(self, neighbor_counts) -> LifeGrid:
was_already_alive = self.cells
twos_and_threes: LifeGrid = {cell for cell, num in neighbor_counts.items()
if num in {2, 3}}
return twos_and_threes & was_already_alive
Test. Green. Commit. What else?
def count_neighbors(self) -> NeighborDict:
neighbors = [(-1, -1), (-1, 0), (-1, 1), (0, -1), (0, 1), (1, -1), (1, 0), (1, 1)]
neighbor_counts: NeighborDict = defaultdict(int)
for x, y in self.cells:
for dx, dy in neighbors:
neighbor_counts[(dx + x, dy + y)] += 1
return neighbor_counts
That list at the top could be a constant.
Neighbors = [(-1, -1), (-1, 0), (-1, 1), (0, -1), (0, 1), (1, -1), (1, 0), (1, 1)]
...
def count_neighbors(self) -> NeighborDict:
neighbor_counts: NeighborDict = defaultdict(int)
for x, y in self.cells:
for dx, dy in Neighbors:
neighbor_counts[(dx + x, dy + y)] += 1
return neighbor_counts
Test, green, commit.
Enough
The as_string method is just used for the Curses display. Possibly it should be moved to that class. Either way, I don’t think we’ll address that today.
Summary
Mostly with automated help from PyCharm (still haven’t turned on the AI and don’t plan to), we’ve made some tiny improvements, and of course the major one of breaking apart the single file into four. For my tiny programs, my approach seems nearly OK but I would say that one time out of four I get into trouble when I go to break things up, circular imports or some weird file/module/package confusion, because I do a very casual project setup, since it’s just me here and I’m not writing anything large or important. Still, I could up my game on that, so I’ll think about that and read up a bit.
A few commits, everything seems nearly good, and the campground is just a bit more clean than it was.
See you next time!