6
\$\begingroup\$

I have used tkinter in Python 3 to create my own version of Conway's Game of Life.

What could I do better in terms of optimisation of speed and memory and PEP8 and cleaning up my code? Also, what can I add to make it more functioning as I only have it generating random maps every time?


My Code (commented very heavily as I want to show this as a project at school):

'''
The universe of the Game of Life is an infinite two-dimensional orthogonal grid of square cells, each of which is in one of two possible states, alive or dead, or "populated" or "unpopulated".
Every cell interacts with its eight neighbours, which are the cells that are horizontally, vertically, or diagonally adjacent.
At each step in time, the following transitions occur:
******************************************************************************************************
 1. Any live cell with fewer than two live neighbours dies, as if caused by underpopulation.
 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 overpopulation.
 4. Any dead cell with exactly three live neighbours becomes a live cell, as if by reproduction.
*******************************************************************************************************
The initial pattern constitutes the seed of the system.
The first generation is created by applying the above rules simultaneously to every cell in the seed—births and deaths occur simultaneously,
and the discrete moment at which this happens is sometimes called a tick (in other words, each generation is a pure function of the preceding one).
The rules continue to be applied repeatedly to create further generations.
'''
from tkinter import *
import random
# Square class: For each cell
class Square:
 # Initialization function (all the precalled things)
 def __init__(self, coords, length, size, state=False, active_col='black', inactive_col='white'):
 self.length = length # Size of map
 self.coords = coords # Top left corner
 self.size = size # Length of one side
 self.state = state # Alive or dead
 self.active_colour = active_col # Colour if alive
 self.inactive_colour = inactive_col # Colour if dead
 # Gives the bottom right values of square
 def rect(self):
 # x+size, y+size
 return (self.coords[0]+self.size, self.coords[1]+self.size)
 # Returns whether a coordinate is inbounds in the grid
 def inbounds(self, coord):
 (x, y) = coord
 # Checks if x value is >= 0 and if the right side of the square is not off the board as x value is top left
 # Checks if y value is >= 0 and if the bottom side of the square is not off the board as y value is top left
 # True or false
 return (x >= 0 and x <= self.length-self.size) and (y >= 0 and y <= self.length-self.size)
 # Returns all the neighbours to the object
 def neighbours(self):
 # self.coords is a tuple. Extracting the x and y of it
 (x, y) = self.coords
 # filter(func, iterable) loops over each value and keeps the value if the function called per value is true.
 # I convert back to list as filter object isn't easy to deal with in my program
 # Each item in the list is dictated by the current x or y +/- size.
 return list(filter(self.inbounds, [
 (x-self.size, y+self.size), (x, y+self.size), (x+self.size, y+self.size),
 (x-self.size, y), (x+self.size, y),
 (x-self.size, y-self.size), (x, y-self.size), (x+self.size, y-self.size),
 ]))
 # Returns a colour whether the object is alive or dead
 def get_colour(self):
 # Short hand if statement
 # If object is alive return alive colour
 # Or else (only two options possible) return dead colour
 return self.active_colour if self.state else self.inactive_colour
# Grid class: The map of each square
class Grid:
 # Initialization function (all the precalled things)
 def __init__(self, length, size, tolerance, active_col='black', inactive_col='white'):
 self.length = length # The length of the map
 self.tolerance = tolerance # The tolerance of generating alive cells randomly
 self.active_col = active_col # Alive colour
 self.inactive_col = inactive_col # Dead colour
 self.squares = self.make_squares(size) # The dictionary of square objects
 # Creates a dictionary of square objects
 def make_squares(self, size):
 # Blank dictionary to add to
 squares = {}
 # (Rows) Loop through the 'length' in steps of 'size' (so as to get the right top left corner each time)
 for y in range(0, self.length, size):
 # (Cells) Loop through the 'length' in steps of 'size' (so as to get the right top left corner each time)
 for x in range(0, self.length, size):
 # If the random float is less than tolerance then make it start dead
 if random.random() < self.tolerance:
 squares[(x, y)] = Square((x, y),
 self.length,
 size,
 active_col=self.active_col,
 inactive_col=self.inactive_col)
 # Otherwise make it alive
 else:
 squares[(x, y)] = Square((x, y),
 self.length,
 size,
 state=True,
 active_col=self.active_col,
 inactive_col=self.inactive_col)
 # Returns a dictionary of squares
 # { coordinate of square: square object }
 return squares
 # Takes a list of coordinates and makes them alive cells
 # Not used but can be used to set alive squares
 def set_squares(self, on_coordinates):
 # Loops through the dictionary of squares
 for coord, square in self.squares:
 # If the square is in the list of coordinates
 if coord in on_coordinates:
 # Square is alive
 square.state = True
 # A set of rules , as defined at the top of this script, to be applied to the grid
 def rules(self):
 # Looping through each square
 for coord, square in self.squares.items():
 # Create a variable to keep track of alive neighbours. Refreshes each square
 alive_neighbours = 0
 # Grab all the squares neighbours
 neighbours = square.neighbours()
 # Loop through each neighbour
 for neighbour in neighbours:
 # If the neighbour is alive
 if self.squares[neighbour].state:
 # Increment the counter of alive neighbours
 alive_neighbours += 1
 # If the square is alive
 if square.state:
 # RULE 1.
 if alive_neighbours < 2:
 # Kill the square
 square.state = False
 # RULE 3.
 elif alive_neighbours > 3:
 # Kill the square
 square.state = False
 # RULE 2.
 else:
 # Keep it alive
 continue
 # If the square isn't alive
 else:
 # RULE 4.
 if alive_neighbours == 3:
 # Bring the square to life
 square.state = True
# App class: the actual tkinter usage
class App:
 # Initialization function (all the precalled things)
 def __init__(self, length, size, tolerance=0.8):
 # length % size NEEDS to = 0
 self.length = length # Length of side of window
 self.size = size # Length of square
 # If the size of the boxes isn't a factor of the window size
 if not self.length % self.size == 0:
 # The boxes don't fit evenly.
 raise Exception("The squares don't fit evenly on the screen." +
 " Box size needs to be a factor of window size.")
 # Create a grid object which can manipulate the squares
 self.grid = Grid(self.length, self.size, tolerance, active_col='#008080', inactive_col='white')
 # tkinter event
 self.root = Tk()
 # Canvas object to display squares
 self.canvas = Canvas(self.root, height=self.length, width=self.length)
 # Set on to the window
 self.canvas.pack()
 # updates canvas
 self.items = self.update_canvas()
 # Creates a loop within the mainloop
 self.root.after(5, self.refresh_screen)
 # Mainloop in tkinter, run the code and loop it until exit called
 self.root.mainloop()
 # Refreshes the screen
 def refresh_screen(self):
 # Applies the rules to the squares
 self.grid.rules()
 # Updates canvas
 self.update_canvas(canvas_done=True, canvas_items=self.items)
 # Reruns the loop
 self.root.after(5, self.refresh_screen)
 # Updates canvas
 def update_canvas(self, canvas_done=False, canvas_items={}):
 # The dict.items() of each square
 # { coord of square: square object }
 square_items = self.grid.squares
 # If the canvas hasn't already been populated with the .create_rect()
 if not canvas_done:
 # Loop through the squares
 for coords, square in square_items.items():
 (b_r_x, b_r_y) = square.rect() # The bottom right coordinates
 (t_l_x, t_l_y) = coords # Top left coordinates
 # Draws a rectangle and stores the data in a dict corresponding to the rectangle drawn
 # Need this to update the rectangles' colours later
 canvas_items[coords] = self.canvas.create_rectangle(t_l_x, t_l_y, b_r_x, b_r_y, fill=square.get_colour())
 # Return the canvas items
 # { coordinates of square drawn: canvas_rectangle object }
 return canvas_items
 # The canvas has already been populated with squares
 # Need this as tkinter doesn't draw on top.
 else:
 # If canvas_items has been specified
 if canvas_items:
 # Loop through the canvas items
 for coords, item in canvas_items.items():
 # Update the canvas to the new colour
 self.canvas.itemconfig(item, fill=square_items[coords].get_colour())
 # No canvas_items so raise a value error
 else:
 # Throws out an error
 raise ValueError("No canvas_items given for re-iterating over canvas squares.")
# If running of the base script and not imported
if __name__ == '__main__':
 # Create an app object
 # Cell Size: higher it is. the faster the computer updates canvas (doesn't matter about amount of cells, just size)
 # ^I don't know why
 app = App(1000, 25, tolerance=0.7)
asked Jan 13, 2018 at 14:49
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Don't do wildcard imports

Use import tkinter as tk and then prefix all tk classes and commands with tk. (eg: tk.Tk(), tk.Frame(...), etc).

PEP8 discourages wildcard imports, and for good reasons. They pollute the global namespace, and they can overwrite variables and classes without you knowing. Tkinter is particularly susceptible to this since both tk and ttk define classes with the same name.

Don't use comments do document a function, use docstrings

Python has a convention for documenting what a function does, docstrings.

Instead of this:

# Returns all the neighbours to the object
def neighbours(self):

... do this:

def neighbours(self):
 """Returns all the neighbours to the object"""

Remove comments that don't add any information

You use a lot of comments that explain things that are obvious, for example:

# tkinter event
self.root = Tk()
...
# Set on to the window
self.canvas.pack()
...
# Updates canvas
def update_canvas(self, canvas_done=False, canvas_items={}):

These sorts of comments have no value, especially if you use good function names. Also, as stated earlier, you should use docstrings in the case of functions.

Move behavior into the Square class

Conceptually, each square represents a single living cell with very well defined behaviors. That is the very definition of an object. Instead of building this behavioral logic into your game itself, you can build it into the Square object. You can do this by creating a tick method (or grow, or mutate or whatever) that implements the rules.

One advantage to this (admittedly, one you might never leverage) is that you can change the game simply by changing the behavior of the Square class; your game logic itself won't have to change at all.

Note: this might be a purely academic exercise. It would likely add a little bit of computational overhead. Still, it would be a good exercise to learn about encapsulating behavior within a class.

answered Jan 13, 2018 at 15:43
\$\endgroup\$
3
  • \$\begingroup\$ Thank you so much! I added all the things mentioned but the last idea. I am confused yet as to what you mean by encapsulating the behaviour: do you mean to move all the rules into the Square class and run then from the grid class? \$\endgroup\$ Commented Jan 14, 2018 at 10:18
  • \$\begingroup\$ @EpicBoss: yes, that's what I mean. Your game logic would be reduced to something like for cell in self.cells: cell.new_generation() \$\endgroup\$ Commented Jan 14, 2018 at 14:51
  • \$\begingroup\$ Thank you I have implemented that and you sure are right that my logic is shorter! However, does this require more computing as I had to make a separate function to call it (the looping of cells is the same right?) ? \$\endgroup\$ Commented Jan 14, 2018 at 16:53
1
\$\begingroup\$

PEP 8 :
All lines should be limited to a maximum of 79 characters.
Docstrings and comments should be limited to 72 characters.
For docstrings use triple double quotes (""" instead of ''').

Names:
Some of your names are confusing. For example, instead of active_col you should write active_color.
Some comments conflict with objects names. For example, in __init__ of Square you write:

self.length = length # Size of map
self.size = size # Length of one side

If you use the names as in your comments, it will be more clear:

self.map_size = map_size
self.side_length = side_length

state could be renamed to is_alive.

Coordinates:
Consider using namedtuple instead of tuple to keep your coordinates. There is example in the docs that fits your case well.

neighbours:
Why don't you take that huge list to a new variable? Also, you could use itertools.product to get neighbours cells coordinates instead of writing all combinations by hand:

x_coordiantes = [x - self.size, x, x + self.size]
y_coordiantes = [y - self.size, y, y + self.size]
neighbours_coordinates = list(itertools.product(x_coordiantes, y_coordiantes))
neighbours_coordinates.remove((x, y))

Or something like that. Probably you could use generators here. You will figure it out.

make_squares:
Instead of writing

for y in range(0, self.length, size):
 for x in range(0, self.length, size):

You can write:

coordinates = itertools.product(range(0, self.length, size), 
 repeat=2)
for x, y in coordinates:

Also, here you write the following two times:

squares[(x, y)] = Square((x, y),
 self.length,
 size,
 active_col=self.active_col,
 inactive_col=self.inactive_col) 

One with state=True and another with implicit False. But why not define state in the if-else statement and write that part from above only once?

state = False if random.random() < self.tolerance else True

You could use numpy.random.choice that supports weights:

state = np.random.choice([False, True],
 p=[self.tolerance, 1 - self.tolerance])

Finally, I don't understand why you save information about coordinates in both keys and values of a dictionary. Looks like a simple list of squares would be enough.

rules:
If you iterate over dictionary and you don't need keys, then use .values() instead of .items().
Variable neighbours is not necessary. Just write for neighbour in square.neighbours():

App:
Why not take number of cells instead of length? In this case you don't need to care about catching the first exception.
In update_canvas you use a mutable default argument. Better don't do this.
You could use dict comprehension for canvas_items.

There is more. But for now this will do.

answered Jan 16, 2018 at 18:12
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.