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

\$\endgroup\$
  • \$\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\$ – Epic Boss Jan 14 '18 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\$ – Bryan Oakley Jan 14 '18 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\$ – Epic Boss Jan 14 '18 at 16:53
0
\$\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.

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