4
\$\begingroup\$

I've just written an implementation of Conway's Game of Life as an exercise and I'd love to get some feedback on how I can improve this program and hopefully my style in general.

I've tested the code in Python 2.7.3. It requires the PyPNG module to render the state of the simulation.

"""Simulate Conway's Game of Life.

The world is finite. Cells at its border always stay dead.
The frames of the simulation are saved as .png files.
"""

from numpy.random import random_integers
from png import from_array

DEAD = 1
ALIVE = 0


def saveAsImage(array, number):
    """Save the array to a .png file.

    The files are saved as 1-bit greyscale (black/white).
    """

    image = from_array(array, 'L', {'bitdepth': 1})
    image.save('image%03i.png' % number)


def simulate(world, simulationLength):
    """Simulate a certain number from steps starting from a given state.

    The frames are saved as .png images in the current working directory.

    Parameters:
        world: The initial state of the world as a two-dimensional numpy
               array in which world[y][x] describes the state of the
               cell at position x, y
        simulationLength: The number of simulated steps,
                          simulationLength + 1 images are created.
    """

    # Retrieve the dimensions of the world
    worldHeight = len(world)
    worldWidth = len(world[0])

    # Make sure there are no living cells on the border.
    world[0] = DEAD  # top
    world[worldHeight - 1] = DEAD  # bottom
    for y in xrange(1, worldHeight - 2):
        world[y][0] = DEAD  # left
        world[y][worldWidth - 1] = DEAD  # right

    saveAsImage(world, 0)

    for frame in xrange(1, simulationLength + 1):
        # Simulate simulationLength frames

        nextWorld = world.copy()

        for y in xrange(1, worldHeight - 1):
            for x in xrange(1, worldWidth - 1):
                # Iterate over all cells within the border

                neighbors = [world[y - 1][x - 1],
                             world[y - 1][x],
                             world[y - 1][x + 1],
                             world[y][x - 1],
                             world[y][x + 1],
                             world[y + 1][x - 1],
                             world[y + 1][x],
                             world[y + 1][x + 1]]
                livingNeighbors = sum(neighbor == ALIVE
                                      for neighbor in neighbors)

                if world[y][x] == ALIVE:
                    # Living cells with less than 2 or more than three
                    # living neighbors die
                    if livingNeighbors not in (2, 3):
                        nextWorld[y][x] = DEAD
                else:
                    # Dead cells with three living neighbors are reborn
                    if livingNeighbors == 3:
                        nextWorld[y][x] = ALIVE

        world = nextWorld

        saveAsImage(world, frame)


if __name__ == '__main__':
    worldWidth = 100
    worldHeight = 100

    # Populate the world with random living/dead (50% each) cells
    world = random_integers(0, 1, (worldHeight, worldWidth))

    simulate(world, 50)
\$\endgroup\$
6
  • \$\begingroup\$ From a quick look, I'd say avoid placing code in the global namespace... place stuff in main() function, and use the __name__ == '__main__' thing. See meta.codereview.stackexchange.com/a/493. \$\endgroup\$
    – tshepang
    Aug 2 '13 at 20:37
  • \$\begingroup\$ Thanks, you're right. My line of reasoning was that I didn't intend this snippet to be imported by another module, but of course it's better to put the simulation into a function instead of hard-coding the parameters like I did. \$\endgroup\$
    – camelNeck
    Aug 2 '13 at 21:32
  • \$\begingroup\$ I've updated my code to reflect your suggestions and already found an error previously covered by namespace ambiguity. \$\endgroup\$
    – camelNeck
    Aug 2 '13 at 22:28
  • \$\begingroup\$ good to know you fixed something as a result of my excellence :) \$\endgroup\$
    – tshepang
    Aug 2 '13 at 22:30
  • 1
    \$\begingroup\$ Instead of world[0] = DEAD don't you need world[0] = [DEAD] * worldWidth ? I don't understand how this can work as it stands. \$\endgroup\$
    – Stuart
    Aug 3 '13 at 11:19
1
\$\begingroup\$

The main thing I want to point is that your algorithm is too simple since it uses matrix (list of lists) as a playing field. If it will be 1000*1000 you will process the field in about a second. And it is already slow. But if 10000*10000? 100 seconds per move (100 millions mostly empty cells to process?)

And these nasty borders? They will spoil many game-related problems.

If you are eager to really improve your game (and perhaps your skills), you can try another implementation.

Store only live cells - a set/list/dict of tuples in form (x, y). There would be interesting little task about searching for neighbor cells - you can find several solutions to do it fast (feel free to write if you want a hint).

Then you will have the game on an endless field and you will like its speed (and you will be proud for your work, I dare to suppose).

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for this valuable suggestion! I'll try to tackle this challenge soon! \$\endgroup\$
    – camelNeck
    Aug 4 '13 at 21:36
2
\$\begingroup\$

Separate different concerns in your code. The function simulate does two things that should be separated, namely changing the state of the world and calling the function that saves it as an image. As it is, it's impossible to re-use only the state-changing logic. Instead, move the game logic to a function simulate_step (or whatever you want to call it).

Stick to the PEP8 naming conventions: functions and variables should be written lowercase_with_underscores. Otherwise, you follow the guide very nicely.

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