3
\$\begingroup\$

Here's my implementation of Game of Life. I'm quite new to Python and would like to know how could I improve that code, especially in terms of performance, compactness and readability.

import copy
import pygame
from pygame import *
import random

#constants
red = (255, 0, 0)
black = (0, 0, 0)
white = (255, 255, 255)
#neighbour coordinates
neighbours = [[-1,-1],[-1,0],[-1,+1],
              [0,-1],        [0,+1],   
              [+1,-1],[+1,0],[+1,+1],]

class cell(object):
        def __init__(self, ngb, state):
                self.state = state
                self.ngb = 0

#2d array for storing cells
cells = [[i for i in range(50)] for i in range(50)]

#random field generation
def generate():
        print "Generating"
        for y in xrange(50):
                for x in xrange(50):
                        cells[x][y] = cell(0, random.randint(0, 1))
        print "DoneGen"

#neighbour processing
def update():
        global cells2
        #saving this turn's state
        cells2=copy.deepcopy(cells)
        for y in xrange(50):
                for x in xrange(50):
                        cellv2=cells2[x][y]
                        cellv2.ngb=0
                        cellv = cells[x][y]
                        #processing 
                        for i in neighbours:
                                #offsetting neighbour coordinates
                                dy=i[0]+y
                                dx=i[1]+x
                                if dy < 0:
                                        dy = 49
                                if dy > 49:
                                        dy = 0
                                if dx < 0:
                                        dx = 49
                                if dx > 49:
                                        dx = 0
                                if cells2[dx][dy].state==1:
                                        cellv2.ngb+=1
                        #updating field
                        if cellv2.state==1 and 2<=cellv2.ngb<=3:
                                cellv.state=1
                        else:
                                cellv.state=0
                        if cellv2.state==0 and cellv2.ngb==3:
                                cellv.state=1

#main game function        
def play():                
        #initialization
        pygame.init()
        scrn = pygame.display.set_mode((500, 500))
        mainsrf = pygame.Surface((500, 500))
        mainsrf.fill(white)
        generate()
        #game cycle
        while 1:
                #tracking quitting
                for event in pygame.event.get():
                        if event.type == QUIT:
                                pygame.quit()
                                sys.exit()
                #drawing
                for y in xrange(50):
                        for x in xrange(50):
                                if cells[x][y].state==1:
                                        pygame.draw.rect(mainsrf, black, (x*10, y*10, 10, 10))
                                else:
                                        pygame.draw.rect(mainsrf, white, (x*10, y*10, 10, 10))
                                if cells[x][y].ngb==3:
                                        pygame.draw.rect(mainsrf, red, (x*10, y*10, 10, 10))
                update()
                scrn.blit(mainsrf, (0, 0))
                pygame.display.update()



#running the game
if __name__ == "__main__":
                                play()
\$\endgroup\$
5
\$\begingroup\$

Functions good practices: Documentation, Parametrization, Single Purpose, Outside State Independence

Documentation

#random field generation
def generate():

Documentation is usually written as a docstring in triple quotes under the function definition (this allows programmatic access via help:

def generate():
    """ Random field generation. """

It also usually is a complete phrase, I would write it a little different:

def generate():
    """ Generates a random game of life board. """

Parametrization

Your function can only generate boards of size \$50 * 50\$, may I be interested in other sizes I would need to modify the definition accordingly.

I suggest asking x and y sizes as parameters:

def generate(x_size, y_size):
        print "Generating"
        for y in xrange(x_size):
                for x in xrange(y_size):
                        cells[x][y] = cell(0, random.randint(0, 1))
        print "DoneGen"

This way you gain more re-usability for your function (and you can test it simpler by giving small sizes).

Single Purpose

    print "Generating"
    ...
    print "DoneGen"

Your function prints to std-out in addition to building the board and this behaviour cannot be turned off. If printing out is not desired the user will not be able to use this function. I would just delete these printing statements after the debugging is complete.

Outside state independence

You need a 2d list called board for this function to work. You may instead create such list and return it.

Write functions to encapsulate logical units of action

wrap_around(dx, dy, x_size, y_size)

                            if dy < 0:
                                    dy = 49
                            if dy > 49:
                                    dy = 0
                            if dx < 0:
                                    dx = 49
                            if dx > 49:
                                    dx = 0

This 8 lines of code provide the program with wrap-around functionality (for example: going too far to the right leaves you all the way back to the left), it would ideal if you wrote a function for this.

next_state(cell, neighbours)

The same can be said for this block of code calculates the next state of a given cell:

                    if cellv2.state==1 and 2<=cellv2.ngb<=3:
                            cellv.state=1
                    else:
                            cellv.state=0
                    if cellv2.state==0 and cellv2.ngb==3:
                            cellv.state=1

decide_colour(cell)

                            if cells[x][y].state==1:
                                    pygame.draw.rect(mainsrf, black, (x*10, y*10, 10, 10))
                            else:
                                    pygame.draw.rect(mainsrf, white, (x*10, y*10, 10, 10))
                            if cells[x][y].ngb==3:
                                    pygame.draw.rect(mainsrf, red, (x*10, y*10, 10, 10))

The 3 options differ only in the colour of the drawing so you could also cut down code duplication by using such a function.

Minor: use ALL_CAPS for constants

As a widely accepted convention constants are written ALL CAPS to quickly discern them from variable variables.

RED = (255, 0, 0)
BLACK = (0, 0, 0)
WHITE = (255, 255, 255)
#neighbour coordinates
NEIGHBOURS = [[-1,-1],[-1,0],[-1,+1],
              [0,-1],        [0,+1],   
              [+1,-1],[+1,0],[+1,+1],]

Give meaningful names: ngb?

self.ngb = 0

I cannot understand what the purpose of the ngb field is even after reading the code.

This is a bad sign indicating that a more descriptive name should be adopted.

\$\endgroup\$
  • \$\begingroup\$ Wouldn't it be easier to make x_size and y_size global variables instead of inputting them into the wrap-around function? \$\endgroup\$ – Mr. Lescum Jun 11 '16 at 14:41
  • \$\begingroup\$ @Mr.Lescum I suggest making them global constants (they would not be changed in the middle of the program). Inputting them in wrap around allows easier testing as your tests won't break when you change these constants. \$\endgroup\$ – Caridorc Jun 11 '16 at 14:44

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.