4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ To create a board… import shelve; b = [[False] * 15] * 15 ; b[7][7] = True ; shelve.open('boardfile.dat')['board'] = b \$\endgroup\$ – 200_success Sep 30 '13 at 21:57
  • \$\begingroup\$ Sections 2 to 5 of my answer to your previous question apply to this question too. \$\endgroup\$ – Gareth Rees Sep 30 '13 at 22:03
2
\$\begingroup\$

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"
\$\endgroup\$
  • 1
    \$\begingroup\$ Shouldn't it be '#' if col else '.' ? \$\endgroup\$ – Josay Oct 1 '13 at 12:20
1
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.