5
\$\begingroup\$

I've just finished with my python implementation of Conway's Game of Life. Can I get some opinions on it?

import pygame
import sys
import time
import random

rules = '''
    1. Any live cell with fewer than two live neighbours dies, as if caused by under-population.
    2. Any live cell with two or three live neighbours lives on to the next generation.
    3. Any live cell with more than three live neighbours dies, as if by overcrowding.
    4. Any dead cell with exactly three live neighbours becomes a live cell, as if by reproduction.'''

#Initializations:

#CONSTANTS
#-------------------
global thesize
thesize = 100
global pixelsize
pixelsize = 10
global FPS
FPS = 100
global windowsizes
windowsize = thesize*pixelsize
global generation
generation = 0
#-------------------

#Define a 2D board (List containing lists)
global board
board = [[False for x in range(thesize)] for x in range(thesize)]

#Set up colors for ease of use
BLACK = (0, 0, 0)
WHITE = (255, 255, 255)

#Set up pygame
pygame.init()
global surface
surface = pygame.display.set_mode((windowsize, windowsize)) # Define the surface for the simulation to run on
pygame.display.set_caption('Conway\'s Game of Life')
surface.fill(WHITE) # Fill the screen white
pygame.display.update()
clock = pygame.time.Clock()

#Function to round to the nearest base
def myround(x, base=5):
    return int(base * round(float(x)/base))

#Function for returning the segment that a number is in
def whichSlot(x, groupsize=pixelsize):
    return x // groupsize

#Function for returning which row and column the mouse is in
def where():
    x, y = pygame.mouse.get_pos()
    return (whichSlot(x), whichSlot(y))
#Function to find number of live neighbors
def neighbors(row, column):
    adjacents = 0

    #Horizontally adjacent
    if row > 0:
        if board[row-1][column]:
            adjacents += 1
    if column > 0:
        if board[row][column-1]:
            adjacents += 1
    if row < thesize-1:
        if board[row+1][column]:
            adjacents += 1
    if column < thesize-1:
        if board[row][column+1]:
            adjacents += 1

    #Diagonally adjacent
    if row > 0 and column > 0:
        if board[row-1][column-1]:
            adjacents += 1
    if row < thesize-1 and column < thesize-1:
        if board[row+1][column+1]:
            adjacents += 1
    if row > 0 and column < thesize-1:
        if board[row-1][column+1]:
            adjacents += 1
    if row < thesize-1 and column > 0:
        if board[row+1][column-1]:
            adjacents += 1

    #Return the final count (0-8)
    return adjacents

#Turn a space of the grid on
def giveLife(ro, col):
    topleft = [ro*pixelsize, col*pixelsize]
    topright = [topleft[0]+pixelsize, topleft[1]]
    botleft = [topleft[0], topleft[1]+pixelsize]
    botright = [topleft[0]+pixelsize, topleft[1]+pixelsize] 
    pygame.draw.polygon(surface, BLACK, [topleft, topright, botright, botleft])

#Turn a space of the grid off
def killRuthlessly(ro, col):
    topleft = [ro*pixelsize, col*pixelsize]
    topright = [topleft[0]+pixelsize, topleft[1]]
    botleft = [topleft[0], topleft[1]+pixelsize]
    botright = [topleft[0]+pixelsize, topleft[1]+pixelsize] 
    pygame.draw.polygon(surface, WHITE, [topleft, topright, botright, botleft])





#Main loop
run = False
while 1:
    #Draw the board as rectangles
    for r in range(len(board)):
        for c in range(len(board)):
            if board[r][c]:
                giveLife(r, c)
            if not board[r][c]:
                killRuthlessly(r, c)

    #Process Events            
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            pygame.quit()
            break;
        if event.type == pygame.KEYDOWN:
            if event.key == pygame.K_ESCAPE:
                pygame.quit()
                break;
            if event.key == pygame.K_RETURN:
                run = not run
            if event.key == pygame.K_c:
                generation = 0
                board = [[False for x in range(thesize)] for x in range(thesize)]

            if event.key == pygame.K_r:
                generation = 0
                possibilities = [False, False, True]
                for r in range(thesize):
                    for c in range(thesize):
                        board[r][c] = random.choice(possibilities)


    #RULES
    if run:
        tempboard = [[False for x in range(thesize)] for x in range(thesize)]
        for r in range(len(board)):
            for c in range(len(board)):
                neighborcount = neighbors(r, c)
                if board[r][c]: #any live cell
                    if neighborcount < 2:
                        tempboard[r][c] = False #dies
                    if neighborcount > 3: #With more than three live neighbors
                        tempboard[r][c] = False #dies
                    if neighborcount == 2 or neighborcount == 3:
                        tempboard[r][c] = True #lives on to the next generation 
                elif not board[r][c]: #any dead cell
                    if neighborcount == 3: #with exactly three live neighbors
                        tempboard[r][c] = True #becomes a live cell
        board = tempboard
        generation += 1

    presses = pygame.mouse.get_pressed()
    if presses[0]:
        putx, puty = where()
        board[putx][puty] = True
        if not run:
            generation = 0
    if presses[2]:
        putx, puty = where()
        board[putx][puty] = False
        if not run:
            generation = 0

    if run:
        clock.tick(FPS)

    pygame.display.flip()
\$\endgroup\$
3
\$\begingroup\$

I'll assume the first few style changes that jonrsharpe mentioned.

I think setting up colours separately to other constants is actually advisable, but you should at least use the pygame.Color class:

BLACK = pygame.Color(0, 0, 0, 255)
WHITE = pygame.Color(255, 255, 255, 255)

or, better, just use pygame.Color("black") and pygame.Color("white").

Your general layout is

 constants
 setup code
 functions
 execution code

but it should look more like

constants
functions
main function:
    setup code
    execution code
call main

This will require you to make surface or board arguments to several functions that currently take it from the global scope.

Your my_round function (renamed according to jonrsharpe's advice) is badly named: the name tells you little about its functionality. You never use it, though, so just chuck it.

Personally I would remove and inline which_slot. It doesn't give either clarity or succinctness. When returning a tuple from where, you don't need brackets. If where is now

def where():
    """Function for returning which row and column the mouse is in"""
    x, y = pygame.mouse.get_pos()
    return x // PIXEL_SIZE, y // PIXEL_SIZE

it's problematic in that it mixes logic (converting coordinates) and fetching input (pygame.mouse.get_pos). Rather, the coordinates should be passed in to the function. The name should also be more descriptive - after this change I would call it real_to_world_coordinates.

The docstring (as per jonrsharpe's advice again), should be more imperative (like a command) rather than a statement ("return", not "function returning").

def real_to_world_coordinates(position):
    """Get world position from screen coordinates"""
    x, y = position
    return x // PIXEL_SIZE, y // PIXEL_SIZE

I found your neighbors function better named as, say, live_neighbors - I am used to treating all adjacent cells as neighbors. The docstring agrees with me - it says "live neighbors". If all neighbors were live, this would be a strange thing to say.

The function has a lot of redundancy. A simpler method would be, say,

def live_neighbors(board, row, column):
    """Count number of live neighbors"""
    neighbours = (
        (-1, -1), (-1,  0), (-1, +1),
        ( 0, -1),           ( 0, +1),
        (+1, -1), (+1,  0), (+1, +1),
    )

    adjacent = 0

    for dx, dy in neighbours:
        if 0 <= row+dx < THE_SIZE and 0 <= column+dy < THE_SIZE:
            adjacent += board[row+dx][column+dy]

    return adjacent

This does come with a speed penalty, but this is dealt with by a fast-path for the centre part:

def live_neighbors(board, row, column):
    """Count number of live neighbors"""
    # Fast path for anything not on the edge
    if 1 <= row < THE_SIZE-1 and 1 <= column < THE_SIZE-1:
        return (
            board[row+1][column+1] +
            board[row+1][column  ] +
            board[row+1][column-1] +
            board[row  ][column+1] +
            board[row  ][column-1] +
            board[row-1][column+1] +
            board[row-1][column  ] +
            board[row-1][column-1]
        )

    neighbours = (
        (-1, -1), (-1,  0), (-1, +1),
        ( 0, -1),           ( 0, +1),
        (+1, -1), (+1,  0), (+1, +1),
    )

    adjacent = 0

    for dx, dy in neighbours:
        if 0 <= row+dx < THE_SIZE and 0 <= column+dy < THE_SIZE:
            adjacent += board[row+dx][column+dy]

    return adjacent

This is much faster than the old one, and also somewhat clearer despite the duplication. One could go half-half to get

def live_neighbors(board, row, column):
    """Count number of live neighbors"""

    neighbours = (
        (-1, -1), (-1,  0), (-1, +1),
        ( 0, -1),           ( 0, +1),
        (+1, -1), (+1,  0), (+1, +1),
    )

    adjacent = 0

    # Fast path for anything not on the edge
    if 1 <= row < THE_SIZE-1 and 1 <= column < THE_SIZE-1:
        for dx, dy in neighbours:
            adjacent += board[row+dx][column+dy]
    else:
        for dx, dy in neighbours:
            if 0 <= row+dx < THE_SIZE and 0 <= column+dy < THE_SIZE:
                adjacent += board[row+dx][column+dy]

    return adjacent

but the speed improvement from unrolling matters after the next step:

You spend a lot of time calling give_life and kill_ruthlessly on squares that haven't updated. Strangely, you also draw before updating. If one cached the old board, one could do

#Draw the board as rectangles
for r in range(len(board)):
    for c in range(len(board)):
        if board[r][c] and not old_board[r][c]:
            give_life(surface, r, c)
        if not board[r][c] and old_board[r][c]:
            kill_ruthlessly(surface, r, c)

and draw far fewer objects.

Further, one should just draw rectangles, not arbitrary polygons. This should also result in fewer graphics errors, which are made more obvious by the above:

def give_life(surface, row, column):
    """Turn a space of the grid on"""
    rect = pygame.Rect(
        row * PIXEL_SIZE,
        column * PIXEL_SIZE,
        PIXEL_SIZE,
        PIXEL_SIZE
    )
    pygame.draw.rect(surface, BLACK, rect)

def kill_ruthlessly(surface, row, column):
    """Turn a space of the grid off"""
    rect = pygame.Rect(
        row * PIXEL_SIZE,
        column * PIXEL_SIZE,
        PIXEL_SIZE,
        PIXEL_SIZE
    )
    pygame.draw.rect(surface, WHITE, rect)

Instead of while 1, use while True.

You initialize the board as

board = [[False for x in range(THE_SIZE)] for x in range(THE_SIZE)]

This may be better as

board = [[False] * THE_SIZE for _ in range(THE_SIZE)]

You have two stray semicolons. Trash 'em.

Your check

if event.type == pygame.QUIT:
    pygame.quit()
    break

Doesn't actually work. You need to fully exit, not just uninitialize SDL. Actually, there isn't much point running pygame.quit at all. I suggest just doing return from a main function.

I personally don't like comments at the end of lines, but if you must at least space them properly (gap of two spaces and a space after the hash).

This is overcomplicated:

if board[r][c]:
    if neighborcount < 2:
        tempboard[r][c] = False
    if neighborcount > 3:
        tempboard[r][c] = False
    if neighborcount == 2 or neighborcount == 3:
        tempboard[r][c] = True
elif not board[r][c]:
    if neighborcount == 3:
        tempboard[r][c] = True

Some ifs are exclusive, so should be elifs, but there's also no point doing

    if neighborcount == 2 or neighborcount == 3:
        tempboard[r][c] = True

and your

elif not board[r][c]

is redundant.

In fact, the whole thing can be just

if board[r][c]:
    tempboard[r][c] = 2 <= neighborcount <= 3
else:
    tempboard[r][c] = neighborcount == 3

The mouse code now looks like

presses = pygame.mouse.get_pressed()
if presses[0]:
    putx, puty = real_to_world_coordinates(pygame.mouse.get_pos())
    board[putx][puty] = True
    if not run:
        generation = 0

if presses[2]:
    putx, puty = real_to_world_coordinates(pygame.mouse.get_pos())
    board[putx][puty] = False
    if not run:
        generation = 0

There is a lot of redundancy. Consider something like

presses = pygame.mouse.get_pressed()
if presses[0] or presses[2]:
    turn_on = presses[0]

    putx, puty = real_to_world_coordinates(pygame.mouse.get_pos())
    board[putx][puty] = turn_on
    if not run:
        generation = 0

Back to speed, one improvement would be to desync framerate from the game state, like

if time.time() - last_frame_time > 1.0/FPS:
    # Draw the board
    for r in range(len(board)):
        for c in range(len(board)):
            if board[r][c] and not old_board[r][c]:
                give_life(surface, r, c)
            if not board[r][c] and old_board[r][c]:
                kill_ruthlessly(surface, r, c)

    pygame.display.flip()
    last_frame_time = time.time()

    old_board = [row[:] for row in board]

This way you can have multiple frames per draw if draws become the expensive part. Unfortunately, the slowest part is again live_neighbors despite the fast path. Let's look at its disassembly. It's long and scary, so hold on tight.

First is the if 1 <= row < THE_SIZE-1 and 1 <= column < THE_SIZE-1 check.

  0 LOAD_CONST               1 (1)
  3 LOAD_FAST                1 (row)
  6 DUP_TOP
  7 ROT_THREE
  8 COMPARE_OP               1 (<=)
 11 JUMP_IF_FALSE_OR_POP    27
 14 LOAD_GLOBAL              0 (THE_SIZE)
 17 LOAD_CONST               1 (1)
 20 BINARY_SUBTRACT
 21 COMPARE_OP               0 (<)
 24 JUMP_FORWARD             2 (to 29)
 27 ROT_TWO
 28 POP_TOP
 29 POP_JUMP_IF_FALSE      208
 32 LOAD_CONST               1 (1)
 35 LOAD_FAST                2 (column)
 38 DUP_TOP
 39 ROT_THREE
 40 COMPARE_OP               1 (<=)
 43 JUMP_IF_FALSE_OR_POP    59
 46 LOAD_GLOBAL              0 (THE_SIZE)
 49 LOAD_CONST               1 (1)
 52 BINARY_SUBTRACT
 53 COMPARE_OP               0 (<)
 56 JUMP_FORWARD             2 (to 61)
 59 ROT_TWO
 60 POP_TOP
 61 POP_JUMP_IF_FALSE      208

Eek. For a quick check, this is a lot of work. Doing a simpler if row and column and row < THE_SIZE-1 and column < THE_SIZE-1 gives

  0 LOAD_FAST                1 (row)
  3 POP_JUMP_IF_FALSE      188
  6 LOAD_FAST                2 (column)
  9 POP_JUMP_IF_FALSE      188
 12 LOAD_FAST                1 (row)
 15 LOAD_GLOBAL              0 (THE_SIZE)
 18 LOAD_CONST               1 (1)
 21 BINARY_SUBTRACT
 22 COMPARE_OP               0 (<)
 25 POP_JUMP_IF_FALSE      188
 28 LOAD_FAST                2 (column)
 31 LOAD_GLOBAL              0 (THE_SIZE)
 34 LOAD_CONST               1 (1)
 37 BINARY_SUBTRACT
 38 COMPARE_OP               0 (<)
 41 POP_JUMP_IF_FALSE      188

which is a bit shorter but not optimal. We can avoid the LOAD_GLOBALs with this idiom:

def live_neighbors(board, row, column, right_edge=THE_SIZE-1):

giving just

  0 LOAD_FAST                1 (row)
  3 POP_JUMP_IF_FALSE      180
  6 LOAD_FAST                2 (column)
  9 POP_JUMP_IF_FALSE      180
 12 LOAD_FAST                1 (row)
 15 LOAD_FAST                3 (right_edge)
 18 COMPARE_OP               0 (<)
 21 POP_JUMP_IF_FALSE      180
 24 LOAD_FAST                2 (column)
 27 LOAD_FAST                3 (right_edge)
 30 COMPARE_OP               0 (<)
 33 POP_JUMP_IF_FALSE      180

which looks good enough for now.

Next is the addition:

 36 LOAD_FAST                0 (board)
 39 LOAD_FAST                1 (row)
 42 LOAD_CONST               1 (1)
 45 BINARY_ADD
 46 BINARY_SUBSCR
 47 LOAD_FAST                2 (column)
 50 LOAD_CONST               1 (1)
 53 BINARY_ADD
 54 BINARY_SUBSCR
 55 LOAD_FAST                0 (board)
 58 LOAD_FAST                1 (row)
 61 LOAD_CONST               1 (1)
 64 BINARY_ADD
 65 BINARY_SUBSCR
 66 LOAD_FAST                2 (column)
 69 BINARY_SUBSCR
 70 BINARY_ADD
 71 LOAD_FAST                0 (board)
 74 LOAD_FAST                1 (row)
 77 LOAD_CONST               1 (1)
 80 BINARY_ADD
 81 BINARY_SUBSCR
 82 LOAD_FAST                2 (column)
 85 LOAD_CONST               1 (1)
 88 BINARY_SUBTRACT
 89 BINARY_SUBSCR
 90 BINARY_ADD
 91 LOAD_FAST                0 (board)
 94 LOAD_FAST                1 (row)
 97 BINARY_SUBSCR
 98 LOAD_FAST                2 (column)
101 LOAD_CONST               1 (1)
104 BINARY_ADD
105 BINARY_SUBSCR
106 BINARY_ADD
107 LOAD_FAST                0 (board)
110 LOAD_FAST                1 (row)
113 BINARY_SUBSCR
114 LOAD_FAST                2 (column)
117 LOAD_CONST               1 (1)
120 BINARY_SUBTRACT
121 BINARY_SUBSCR
122 BINARY_ADD
123 LOAD_FAST                0 (board)
126 LOAD_FAST                1 (row)
129 LOAD_CONST               1 (1)
132 BINARY_SUBTRACT
133 BINARY_SUBSCR
134 LOAD_FAST                2 (column)
137 LOAD_CONST               1 (1)
140 BINARY_ADD
141 BINARY_SUBSCR
142 BINARY_ADD
143 LOAD_FAST                0 (board)
146 LOAD_FAST                1 (row)
149 LOAD_CONST               1 (1)
152 BINARY_SUBTRACT
153 BINARY_SUBSCR
154 LOAD_FAST                2 (column)
157 BINARY_SUBSCR
158 BINARY_ADD
159 LOAD_FAST                0 (board)
162 LOAD_FAST                1 (row)
165 LOAD_CONST               1 (1)
168 BINARY_SUBTRACT
169 BINARY_SUBSCR
170 LOAD_FAST                2 (column)
173 LOAD_CONST               1 (1)
176 BINARY_SUBTRACT
177 BINARY_SUBSCR
178 BINARY_ADD
179 RETURN_VALUE

This is evidently too much, so some thought should go into fixing this. Perhaps caching some things might be better:

left, mid, right = board[row-1:row+2]
up_idx = column + 1
dn_idx = column - 1
return (
    right[up_idx] +
    right[column] +
    right[dn_idx] +
    mid  [up_idx] +
    mid  [dn_idx] +
    left [up_idx] +
    left [column] +
    left [dn_idx]
)

giving

 36 LOAD_FAST                0 (board)
 39 LOAD_FAST                1 (row)
 42 LOAD_CONST               1 (1)
 45 BINARY_SUBTRACT
 46 LOAD_FAST                1 (row)
 49 LOAD_CONST               2 (2)
 52 BINARY_ADD
 53 BUILD_SLICE              2
 56 BINARY_SUBSCR
 57 UNPACK_SEQUENCE          3
 60 STORE_FAST               4 (left)
 63 STORE_FAST               5 (mid)
 66 STORE_FAST               6 (right)
 69 LOAD_FAST                2 (column)
 72 LOAD_CONST               1 (1)
 75 BINARY_ADD
 76 STORE_FAST               7 (up_idx)
 79 LOAD_FAST                2 (column)
 82 LOAD_CONST               1 (1)
 85 BINARY_SUBTRACT
 86 STORE_FAST               8 (dn_idx)
 89 LOAD_FAST                6 (right)
 92 LOAD_FAST                7 (up_idx)
 95 BINARY_SUBSCR
 96 LOAD_FAST                6 (right)
 99 LOAD_FAST                2 (column)
102 BINARY_SUBSCR
103 BINARY_ADD
104 LOAD_FAST                6 (right)
107 LOAD_FAST                8 (dn_idx)
110 BINARY_SUBSCR
111 BINARY_ADD
112 LOAD_FAST                5 (mid)
115 LOAD_FAST                7 (up_idx)
118 BINARY_SUBSCR
119 BINARY_ADD
120 LOAD_FAST                5 (mid)
123 LOAD_FAST                8 (dn_idx)
126 BINARY_SUBSCR
127 BINARY_ADD
128 LOAD_FAST                4 (left)
131 LOAD_FAST                7 (up_idx)
134 BINARY_SUBSCR
135 BINARY_ADD
136 LOAD_FAST                4 (left)
139 LOAD_FAST                2 (column)
142 BINARY_SUBSCR
143 BINARY_ADD
144 LOAD_FAST                4 (left)
147 LOAD_FAST                8 (dn_idx)
150 BINARY_SUBSCR
151 BINARY_ADD
152 RETURN_VALUE

These help timings, but only by a small amount (20-30%). (I'm not entirely sure much was gained from looking at the disassembly; there wasn't much to fix.)

We want better than that, so let's try something else. Perhaps one can increase calculation speed by performing the operation on the whole map. Something like

def live_neighbors(board):
    # return board of counts

This will reduce function call overhead, but it also lets us remove some of the significant calculation overheads. One could do:

def live_neighbors(board):
    """Count number of live neighbors"""
    counts = [[0 for _ in row] for row in board]

    for i in range(THE_SIZE-1):
        ip1 = i + 1
        for j in range(THE_SIZE):
            if board[ip1][j]: counts[i][j] += 1
            if board[i][j]: counts[ip1][j] += 1
            if board[j][ip1]: counts[j][i] += 1
            if board[j][i]: counts[j][ip1] += 1

    for i in range(THE_SIZE-1):
        ip1 = i + 1
        for j in range(THE_SIZE-1):
            jp1 = j + 1

            if board[ip1][jp1]: counts[i][j] += 1
            if board[i][jp1]: counts[ip1][j] += 1
            if board[ip1][j]: counts[i][jp1] += 1
            if board[i][j]: counts[ip1][jp1] += 1

    return counts

This does each direction separately, adding them into an integer board. Alternatively, one could define the board in Numpy and use a convolution, which would be much faster but less "pure".

At this point, it takes about 8.5 seconds for CPython 3 to run 1000 iterations on my computer. PyPy takes just 1 second, or less.

It's still possible to go faster, though! A lot of time is spent rerunning live_neighbors when only small updates have been made. Perhaps you can use that to optimize out calls to live_neighbors? There are even faster strategies, such as Hashlife. These, though, are outside of the scope of this review.

Here's the updated code:

"""
Pythonic implementation of Conway's Game of Life.

Rules:
  1. Any live cell with fewer than two live neighbours dies, as if caused
    by under-population.
  2. Any live cell with two or three live neighbours lives on to the next
    generation.
  3. Any live cell with more than three live neighbours dies, as if by
    overcrowding.
  4. Any dead cell with exactly three live neighbours becomes a live cell,
    as if by reproduction.
"""


import random
import sys
import time

import pygame

# Initializations:
FPS = 100
PIXEL_SIZE = 10
THE_SIZE = 100
WINDOW_SIZE = THE_SIZE * PIXEL_SIZE

BLACK = pygame.Color("black")
WHITE = pygame.Color("white")


def real_to_world_coordinates(position):
    """Get world position from screen coordinates"""
    x, y = position
    return x // PIXEL_SIZE, y // PIXEL_SIZE

def live_neighbors(board):
    """Count number of live neighbors"""
    counts = [[0 for _ in row] for row in board]

    for i in range(THE_SIZE-1):
        ip1 = i + 1
        for j in range(THE_SIZE):
            if board[ip1][j]: counts[i][j] += 1
            if board[i][j]: counts[ip1][j] += 1
            if board[j][ip1]: counts[j][i] += 1
            if board[j][i]: counts[j][ip1] += 1

    for i in range(THE_SIZE-1):
        ip1 = i + 1
        for j in range(THE_SIZE-1):
            jp1 = j + 1

            if board[ip1][jp1]: counts[i][j] += 1
            if board[i][jp1]: counts[ip1][j] += 1
            if board[ip1][j]: counts[i][jp1] += 1
            if board[i][j]: counts[ip1][jp1] += 1

    return counts

def give_life(surface, row, column):
    """Turn a space of the grid on"""
    rect = pygame.Rect(
        row * PIXEL_SIZE,
        column * PIXEL_SIZE,
        PIXEL_SIZE,
        PIXEL_SIZE
    )
    pygame.draw.rect(surface, BLACK, rect)

def kill_ruthlessly(surface, row, column):
    """Turn a space of the grid off"""
    rect = pygame.Rect(
        row * PIXEL_SIZE,
        column * PIXEL_SIZE,
        PIXEL_SIZE,
        PIXEL_SIZE
    )
    pygame.draw.rect(surface, WHITE, rect)

def main():
    #Define a 2D board (List containing lists)
    board = [[False for _ in range(THE_SIZE)] for _ in range(THE_SIZE)]
    old_board = [row[:] for row in board]

    generation = 0

    #Set up pygame
    pygame.init()
    surface = pygame.display.set_mode((WINDOW_SIZE, WINDOW_SIZE)) # Define the surface for the simulation to run on
    pygame.display.set_caption('Conway\'s Game of Life')
    surface.fill(WHITE) # Fill the screen white
    pygame.display.update()
    last_frame_time = time.time()
    t = time.time()

    run = False
    while True:
        # Apply rules
        if run:
            neighborcounts = live_neighbors(board)

            for r in range(THE_SIZE):
                for c in range(THE_SIZE):
                    if board[r][c]:
                        board[r][c] = 2 <= neighborcounts[r][c] <= 3
                    else:
                        board[r][c] = neighborcounts[r][c] == 3

            generation += 1

        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                return

            if event.type == pygame.KEYDOWN:
                if event.key == pygame.K_ESCAPE:
                    return

                if event.key == pygame.K_RETURN:
                    run = not run

                if event.key == pygame.K_c:
                    generation = 0
                    board = [[False] * THE_SIZE for _ in range(THE_SIZE)]

                if event.key == pygame.K_r:
                    generation = 0
                    t = time.time()
                    possibilities = [False, False, True]
                    for r in range(THE_SIZE):
                        for c in range(THE_SIZE):
                            board[r][c] = random.choice(possibilities)

        presses = pygame.mouse.get_pressed()
        if presses[0] or presses[2]:
            turn_on = presses[0]

            putx, puty = real_to_world_coordinates(pygame.mouse.get_pos())
            board[putx][puty] = turn_on
            if not run:
                generation = 0

        if not generation % 100:
            print(generation)
            print(time.time() - t)

        if time.time() - last_frame_time > 1.0/FPS:
            # Draw the board
            for r in range(len(board)):
                for c in range(len(board)):
                    if board[r][c] and not old_board[r][c]:
                        give_life(surface, r, c)
                    if not board[r][c] and old_board[r][c]:
                        kill_ruthlessly(surface, r, c)

            pygame.display.flip()
            last_frame_time = time.time()

            old_board = [row[:] for row in board]

if __name__ == '__main__':
    main()
\$\endgroup\$
  • \$\begingroup\$ Accepted your answer as it is evident you put a LOT of time into it, and it adresses more than style things (although those were very helpful) \$\endgroup\$ – Luke Taylor Mar 31 '15 at 1:08
5
\$\begingroup\$

There are quite a few comments relating to the style guide. If you are following a different one, please include a link to it in your question. So, from the top:


imports should be in alphabetical order, and third-party should be separated from standard library:

import random
import sys
import time

import pygame

Information about the module should be in a module-level docstring (and the line length limit still applies), e.g.:

"""Pythonic implementation of Conway's Game of Life.

Rules:
  1. Any live cell with fewer than two live neighbours dies, as if caused
    by under-population.
  2. Any live cell with two or three live neighbours lives on to the next
    generation.
  3. Any live cell with more than three live neighbours dies, as if by
    overcrowding.
  4. Any dead cell with exactly three live neighbours becomes a live cell,
    as if by reproduction.

"""

If you ever need to access this string, you can do so via __doc__.


Constants should be UPPERCASE_WITH_UNDERSCORES. Also, there's no need for global - it doesn't do anything here, and isn't necessary for read-only access. Note that I have moved in BLACK and WHITE, as these are also constant, and rearranged to alphabetical order (this is a personal preference; it makes it easier to find things as the list gets longer).

BLACK = (0, 0, 0)
FPS = 100
PIXEL_SIZE = 10
THE_SIZE = 100
WHITE = (255, 255, 255)
WINDOW_SIZE = THE_SIZE * PIXEL_SIZE

generation is not constant, so should not be with these other values (also, it doesn't seem to actually do anything).


It is conventional to indicate values that won't be used with an underscore, i.e.:

board = [[False for _ in range(THE_SIZE)] for _ in range(THE_SIZE)]

Function names should lowercase_with_underscores, or at the very least consistent (why myround but whichSlot?) Functions should also have docstrings explaining what they do, what parameters are expected, etc. In some cases you have comments above the function definition; move these inside the function and switch to multiline strings, e.g.:

#Turn a space of the grid on
def giveLife(ro, col): 
    ...

becomes:

def giveLife(ro, col):
    """Turn a space of the grid on."""
    ...

The current global board should be encapsulated in a class. This should also hold the logic to implement the rules and draw the board, rather than have this part of the main GUI loop. Rather than drawing each square as you giveLife and killRuthlessly, have a single method draw that iterates over the board instance attribute and creates each square accordingly. You can also have a simple __str__ method that allows you to look at the board directly, to easily test the simulation separately from the GUI.


The GUI loop itself should be in a function, rather than running at the top level of the module. You can then call this function like:

if __name__ == '__main__':
    run_gui()

to prevent anything happening if you later import this functionality elsewhere. generations should be managed entirely within run_gui, rather than existing as global state.


You have the logic for creating a new, random starting point buried deep inside the GUI loop; instead, if you had a GameOfLife class, you could create a class method to do this in a much more transparent manner. Alternatively, add an instance method to randomly reset the board for an existing instance.


Overall, I would suggest a structure like:

"""Module docstring"""

# import statements

# CONSTANTS

class GameOfLife(object):
    """Hold the state of a single run of the Game of Life."""

    def __init__(self, size=THE_SIZE):
        """Create the starting board as an instance attribute."""
        ...

    def __str__(self):
        """Print out the board as a simple array of integers."""
        ...

    def step(self):
        """Run one step of the simulation, updating the board."""
        ...

    def draw(self, surface):
        """Draw the board's squares on the supplied surface."""
        ...

    @classmethod
    def create_random(cls, size=THE_SIZE):
        """Create a new instance and set up the board randomly."""
        ...


def run_gui(game):
    """Run the GUI loop with the supplied game."""
    # Initialise pygame and the surface
    while True:
        for event in pygame.event.get():
            # Handle any events
            ...
            if event.type == pygame.KEYDOWN:
                ...
                if event.key == pygame.K_r:
                    game = GameOfLife.create_random()
        game.draw(surface)
        game.step()
        clock.tick(FPS)
        pygame.display.flip()

if __name__ == '__main__':
    run_gui(GameOfLife())

If GameOfLife.step starts to get a bit too long, you can always split out some helper methods (e.g. GameOfLife._count_neighbours, etc.; note the "private-by-convention" leading underscore on the method name).

\$\endgroup\$
  • \$\begingroup\$ '(global) isn't necessary for read-only access'. If local name shadows global one? \$\endgroup\$ – Nizam Mohamed Mar 30 '15 at 10:43
  • 1
    \$\begingroup\$ @NizamMohamed If a local name shadows a global one, then fix your scoping. \$\endgroup\$ – Veedrac Mar 30 '15 at 10:46
  • \$\begingroup\$ @NizamMohamed in that case, global won't help anyway - one identifier can only reference one object in one scope. \$\endgroup\$ – jonrsharpe Mar 30 '15 at 10:47
  • 1
    \$\begingroup\$ @LukeTaylor you could look into using dictionaries more, e.g. mapping {(row, col): neighours, ...}; that would speed things up a bit. \$\endgroup\$ – jonrsharpe Mar 30 '15 at 13:26
  • 1
    \$\begingroup\$ @LukeTaylor then you should just make generation a GameOfLife instance attribute (i.e. set self.generation = 0 in __init__). \$\endgroup\$ – jonrsharpe Mar 30 '15 at 13:38
1
\$\begingroup\$

You don't have to declare a variable as global which is already in module's golbal namespace. golbal is used in non-global scope to mean the referred name is in global namespace.

board = [[False for x in range(thesize)] for x in range(thesize)] can be done as board = [[False] * thesize for _ in range(thesize)].

These multiple ifs,

if row > 0:
    if board[row-1][column]:
        adjacents += 1  

can be shortened as,

if row > 0 and board[row-1][column]:
    adjacents += 1

Because we have common values in comparison like these,

if row > 0 and column > 0:
    if board[row-1][column-1]:
        adjacents += 1

comparison can be chained as follows,

if row > 0 < column and board[row-1][column-1]:
    adjacents += 1

The board size doesn't seem to change. Why call len?

for r in range(len(board)):
    for c in range(len(board)):
        if board[r][c]:
            giveLife(r, c)
        if not board[r][c]:
            killRuthlessly(r, c)

len(board) can be replaced with thesize,

for r in range(thesize):
    for c in range(thesize):
        if board[r][c]:
            giveLife(r, c)
        else:
            killRuthlessly(r, c)

for event in pygame.event.get(): in this loop if events and keys are mutually exclusive, must use else otherwise every if will be checked for truthness.

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