3
\$\begingroup\$

This is my first project in pygame so I am open to suggestions how I can improve my python/pygame skills. I have started programming in python couple of weeks ago and I really want to know what I am doing wrong/harder way.

Main game file

import pygame
from time import sleep
from square import square, superior
import copy

pygame.init()

#Colors
background = (0, 0, 0)
deadCell = (195, 200, 181)
aliveCell = (0, 27, 9)

#Can change those if you want
amount = 20
size = 20

#Some math to calculate size of the screen based on size and amount of 
cells + space between them
display_size = ((amount*size) + amount - 1, (amount*size) + amount - 1) 
display = pygame.display.set_mode(display_size)

exitGame = False
gameStarted = False

grid = list()

#FPS control variables
FPS = 2
clock = pygame.time.Clock()

def create_array():
    global grid

    for row in range(0, amount):
        placeholder = []
        for column in range(0, amount):
            cell = square(row, column, column*size + column, row*size + row, amount, size)
            placeholder.append(cell)
        grid.append(placeholder)

def create_grid():
    display.fill(background)
    #Render grid
    for row in grid:
        for cell in row:
            if cell.alive:
                pygame.draw.rect(display, aliveCell, [cell.x, cell.y, size, size])
            else:
            pygame.draw.rect(display, deadCell, [cell.x, cell.y, size, size])
pygame.display.update()

def check():
    global grid

    #Create old gird tamplate
    old_grid = copy.deepcopy(grid)

    for row in grid:
        for cell in row:
            alive = 0

            next_row = cell.row + 1
            previous_row = cell.row - 1

            next_column = cell.column + 1
            previous_column = cell.column - 1

            # To prevent calling old_grid[-1]
            if previous_row < 0:
                previous_row = len(row) + 100

            if previous_column < 0:
                previous_column = len(row) + 100

            #Check for alive neighbors
            try:
                if old_grid[next_row][cell.column].alive:
                    alive += 1
            except:
                pass
            try:
                if old_grid[previous_row][cell.column].alive:
                    alive += 1
            except:
                pass
            try:
                if old_grid[cell.row][previous_column].alive:
                    alive += 1
            except:
                pass
            try:
                if old_grid[cell.row][next_column].alive:
                    alive += 1
            except:
                pass
            try:
                if old_grid[previous_row][previous_column].alive:
                    alive += 1
            except:
                pass
            try:
                if old_grid[previous_row][next_column].alive:
                    alive += 1
            except:
                pass
            try:
                if old_grid[next_row][previous_column].alive:
                    alive += 1
            except:
                pass
            try:
                if old_grid[next_row][next_column].alive:
                    alive += 1
            except:
                pass

            if alive < 2 or alive > 3:
                cell.alive = False
            elif alive == 3:
                cell.alive = True

#Initialize grid list and render first grid view
create_array()
create_grid()

while not exitGame:

    for event in pygame.event.get():
        #Check if user wants to quit
        if event.type == pygame.QUIT:
            exitGame = True

        #Placing cells
        if event.type == pygame.MOUSEBUTTONDOWN and not gameStarted:
            for row in grid:
                for cell in row:
                    click = pygame.mouse.get_pos()
                    if cell.is_in_range(click[0], click[1]):
                        cell.alive = (not cell.alive)

        #Start the game
        if event.type == pygame.KEYDOWN:
            if event.key == pygame.K_SPACE:
                gameStarted = True

    if gameStarted: 
        check()

        #Check if all cells are dead
        if superior().number_alive == 0:
            exitGame = True

    #Render grid
    create_grid()   
    clock.tick(FPS)


pygame.quit()
quit()

Square.py file

class superior():
    number = 0
    number_alive = 0

class square(superior):

    number = 0

    def __init__(self, row, column, x, y, number_of_columns, size):
        self.row = row
        self.column = column
        self.x = x
        self.y = y
        self.number_of_columns = number_of_columns
        self.size = size
        self.live = False

        superior.number += 1

    def is_in_range(self, x, y):
        if x in range(self.x, self.x + self.size + 1) and y in range(self.y, self.y + self.size + 1):
            return True
        else:
            return False

    @property
    def alive(self):
        return self.live

    @alive.setter
    def alive(self, isalive):
        if self.live and not isalive:
            superior.number_alive -= 1
        elif not self.live and isalive:
            superior.number_alive += 1

        self.live = isalive
\$\endgroup\$
  • 2
    \$\begingroup\$ As I always tell programmers who are implementing Game of Life for the first time: you owe it to yourself to learn Gosper's Algorithm. It not only allows you to easily compute trillions of boards per second that are trillions of cells -- far larger than you'd think would fit into memory -- but it will change the way you think about programming. It gets its wins by making clever use of functional programming techniques. \$\endgroup\$ – Eric Lippert Dec 3 '18 at 23:18
  • \$\begingroup\$ I will look into that and maybe rewrite my implementation afterwards, thanks! \$\endgroup\$ – Jacob233 Dec 5 '18 at 13:31
2
\$\begingroup\$

I've never used pygame, but I can make suggestions regarding the rest of your code.


In your main file, you have two functions, both of which contain the line global grid at the top. Since some external grid is a dependency of the function, it should be explicitly passed in as a parameter instead:

global_grid = []

def create_array(grid):
    for row in range(0, amount):
    ...

create_array(global_grid)

Why? Now create_array can be tested entirely independently of anything else going on. You can create some test grid to do testing with in a REPL, pass the grid to create_array, and have it do its thing. With how you have things set up now though, to do any testing you need to actually alter the code by changing the global grid.

If you look at create_array, why does it even need to know about grid though? All it's doing is appending data to it; and it seems likely that it's assuming grid is already empty. It would make much more sense to have something like this where it just has its own grid:

# Arguably, "amount" should be a parameter as well
def create_array():
    grid = [] # No more global reference, instead create a local

    for row in range(0, amount):
        placeholder = [] # "row# may be a more appopriate name
        for column in range(0, amount):
            cell = square(row, column, column*size + column, row*size + row, amount, size)
            placeholder.append(cell)
        grid.append(placeholder)

    return grid # Now return the local

global_grid = create_array() # Assign the return of the function

Ideally, each function should explicitly take as parameters any data that it needs to operate, and explicitly return any data that represents its purpose. With this new version, the function can be tested by running it and examining the output in the REPL. You aren't needlessly altering any global data that other function calls later on may be effected by.

And the same goes for check. What if you want to test its checking ability by throwing random data at it? If all your functions rely on the same global, and you're altering that global for every test, you're also potentially changing the behavior of any other function that's relying on the global grid. Change it to:

def check(grid):
    #Create old gird tamplate
    old_grid = copy.deepcopy(grid)
    ...

And pass in the grid you want checked when you want to call it.

I'll just mention as an aside that the expensive copy.deepcopy(grid) call can be entirely avoided if you use immutable data structures. This is a prime case where immutability is superior. In languages like Clojure and Haskell that use immutable structures as the default, creating copies of data is very cheap. When I write a Conway's Game of Life implementation in Clojure, I never have to manually create a deep copy of the grid, because every time I "alter" the grid, I'm making a cheap copy of it automatically. Unfortunately, I don't think Python has very good support for immutable data structures (unless a good library is available, which I wouldn't doubt actually). It's something to keep in mind though when working on projects like this.


try:
    if old_grid[cell.row][previous_column].alive:
        alive += 1
except:
    pass

This is literally one of the worst things you could possibly do. In this particular case, only a limited number of things can go wrong, but you are setting yourself up for excruciating debugging if you completely throw away any error you may receive.

Let's say in the future you add a function call in there, but you haven't sufficiently tested the new function. One day, it breaks due to some unforeseen circumstance, and throws an exception. Normally, the exception would cause the program to crash, it tells you what's wrong, you read the error and fix the problem, and get it working again. With how you have it now, try...except: pass will silence any error that you receive; regardless of its source or reason. As someone that's been bit by exception swallowing before (via Clojure's futures), I can tell you it is the worst debugging and programming experiences you can have. Everything suddenly stops working in weird ways, yet you have no errors indicating what's wrong. It's awful. Don't do that to yourself.

I'm assuming you're doing this to simplify out of bounds checks. Using exceptions to handle bounds checks will likely be slower that the alternative of just not checking out of bounds cells in the first place. I'm going into brain lock on how to better approach this in Python, but iterating over the indices of the grid and using the min and max functions that account for the width/height of the grid would be better. You could also create a is_inbounds function that returns if a given coordinate is valid in the grid or not, and use the check.

At the very least, if you wanted to keep using the "ask for forgiveness instead of permission" approach, you should specify what type of exception you're catching:

try:
    if old_grid[cell.row][previous_column].alive:
        alive += 1

except IndexError as e: # Specify that you only want to catch IndexErrors
    pass

def is_in_range(self, x, y):
  if x in range(self.x, self.x + self.size + 1) and y in range(self.y, self.y + self.size + 1):
    return True
  else:
    return False

Would be more succinctly written as

def is_in_range(self, x, y):
  return x in range(self.x, self.x + self.size + 1) and
         y in range(self.y, self.y + self.size + 1)

Or, to take advantage of Python allowing comparison chaining:

def is_in_range(self, x, y):
  return (self.x <= x <= self.size) and
         (self.y <= y <= self.size)

if event.type == pygame.KEYDOWN:
    if event.key == pygame.K_SPACE:
        gameStarted = True

Is the same as:

if event.type == pygame.KEYDOWN and event.key == pygame.K_SPACE:
    gameStarted = True

If you had an elif or else after one of the condtions and needed to handle false cases, it would be different. Here though, you only care if they're both true (and).


Those are the major things that stuck out to me. I hope that helped.

\$\endgroup\$
  • \$\begingroup\$ I really apreciate that you spend a lot of time writing it. I know that try/except statmetn isn't the best option, my first aporach was with if statments but I had like 20 ifs for each situation but it was very unredable so I decided to rewrite it with try/except. I will analyze your sugestions and rewrite my program to be more object oriented and get rid of global variables. \$\endgroup\$ – Jacob233 Dec 5 '18 at 13:46

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.