Game Of Life OOP Python

Question

This is pretty much an OOP version of this question, with the improvements:

import copy
import shelve
class GameOfLife(object):
 def __init__(self, board, generations = 10):
 self.__board = board
 for i in range(generations):
 print(self)
 self.__nextGeneration()
 
 def __str__(self):
 string = ""
 for row in self.__board:
 for cell in row:
 if cell:
 string += "#"
 else:
 string += "."
 string += "\n"
 return string
 
 def __isInRange(self, row, cell):
 return 0 <= row < len(self.__board) and \
 0 <= cell < len(self.__board[0])
 
 def __countSurrounding(self, row, cell):
 SURROUNDING = ((row - 1, cell - 1),
 (row - 1, cell ),
 (row - 1, cell + 1),
 (row , cell - 1),
 (row , cell + 1),
 (row + 1, cell - 1),
 (row + 1, cell ),
 (row + 1, cell + 1))
 count = 0
 for surrRow, surrCell in SURROUNDING:
 if self.__isInRange(surrRow, surrCell) and \
 self.__board[surrRow][surrCell]:
 count += 1
 return count
 
 def __nextGeneration(self):
 nextBoard = copy.deepcopy(self.__board)
 for row in range(len(self.__board)):
 for cell in range(len(self.__board[0])):
 if self.__board[row][cell] and \
 self.__countSurrounding(row, cell) not in (2, 3):
 nextBoard[row][cell] = 0
 elif not self.__board[row][cell] and \
 self.__countSurrounding(row, cell) == 3:
 nextBoard[row][cell] = 1
 self.__board = nextBoard
 
def main():
 boardFile = shelve.open("boardFile.dat")
 board = boardFile["board"]
 game = GameOfLife(board)
 
if __name__ == "__main__":
 main()

Where boardFile.dat is just a shelved file containing a boolean array.

Can anyone provide an honest code review? Thanks.

• 200_success: To create a board... import shelve; b = [[False] * 15] * 15 ; b[7][7] = True ; shelve.open('boardfile.dat')['board'] = b
• Gareth Rees: Sections 2 to 5 of my answer to your previous question apply to this question too.

Answer 1

Object-Orientedness

I find your object-oriented design deficient. Basically, it's a bunch of procedural code that has been thrown into a class. But what can you do with the GameOfLife object that you instantiate? Nothing!

I would split the functionality into a Board class and a GameOfLife class. The Board should be able to provide its own string representation, report the contents of a cell, set the contents of a cell, check if a coordinate exists, clone itself, and perhaps load from or save to a file. The GameOfLife class would be a generator; each time you call life.next() it would produce a new board.

Your main() function should look like this:

board = Board.load('boardFile.dat')
game = GameOfLifeGenerator(board)
for _ in range(11):
 print(board)
 board = game.next()

Alternatively,

from itertools import islice
board = Board(15)
board.fill(row=7, col=7)
for state in islice(GameOfLifeGenerator(board), 11):
 print(state)

Naming

You shouldn't be using double-underscore variables. If you want to suggest that an instance variable is private, use a name with a single underscore.

You use cell to mean column number, which is confusing terminology. A "cell" should refer to one of the points on the grid; a cell has row and column coordinates. (In contrast, I don't object as much to using row as shorthand for the coordinate of a row. English is just weird that way.)

In __countSurrounding(), the SURROUNDING list shouldn't be all caps. I'd consider that to be a variable rather than a constant. (You could define it as SURROUNDING_OFFSETS = ((-1, -1), (-1, 0), ...) instead, which would be a constant.) Also consider laying out the code this way for quick visualization:

surrounding = ((row - 1, col - 1), (row - 1, col), (row - 1, col + 1),
 (row , col - 1), (row , col + 1),
 (row + 1, col - 1), (row + 1, col), (row + 1, col + 1))

Miscellaneous

In __isInRange() and __nextGeneration(), you use len(__board[0]) as the width. I would prefer that you use len(__board(row)) instead.

The __str__() function could be implemented more succinctly as

def __str__(self):
 return "\n".join(
 [''.join(
 ['#' if cell else '.' for cell in row]
 ) for row in self._board]
 ) + "\n"

or

def __str__(self):
 return "\n".join(
 map(lambda row: ''.join(
 map(lambda cell: '#' if cell else '.', row)
 ), self._board)
 ) + "\n"

• SylvainD: Shouldn't it be '#' if col else '.' ?

Answer 2

This looks good. Here are a few (untested) comments.

• Your __str__ method could take advantage of ternary operator, list comprehension and the join method :

This :

def __str__(self):
 string = ""
 for row in self.__board:
 for cell in row:
 if cell:
 string += "#"
 else:
 string += "."
 string += "\n"
 return string

becomes this,

def __str__(self):
 string = ""
 for row in self.__board:
 for cell in row:
 string += "#" if cell else "."
 string += "\n"
 return string

this :

def __str__(self):
 string = ""
 for row in self.__board:
 string += "".join(["#" if cell else "." for cell in row])
 string += "\n"
 return string

and ultimately this :

def __str__(self):
 return "\n".join(["".join(["#" if cell else "." for cell in row]) for row in self.__board])

• Don't iterate using range(len(list)), use enumerate.

For example :

for i,row in enumerate(self.__board):
 for j,cell in enumerate(row):
 if cell:
 if self.__countSurrounding(i, j) not in (2, 3):
 nextBoard[i][j] = 0
 else:
 if self.__countSurrounding(i, j) == 3:
 nextBoard[i][j] = 1

• You could define SURROUNDING as a constant expression.

You just need to a bit of the code using it.

 SURROUNDING = ((-1, -1),
 (-1, 0),
 (-1, +1),
 ( 0, -1),
 ( 0, +1),
 (+1, -1),
 (+1, 0),
 (+1, +1))
 count = 0
 for x,y in SURROUNDING:
 i,j = row+x, cell+y
 if self.__isInRange(i,j) and \
 self.__board[i][j]:
 count += 1