conway's game of life / Single Responsibility Principle

[hanscvanleeuwen]

While i've finished my homework : write a simple game in less than 80 lines (see my previous topic, for which I got a C  , with comment that the key binding solution was ingenious, thnx Mekire )
The suggestions I got from both this forum and my instructor was to write the program logic up into more functions. Every function should have a single job. Although beyond my current homework goal, I've now added Class Square and Class Neighbours to the program. Does the program now seems logical ? (two while loops and break from first) or should it be broken up in even more classes/definitions ?
HCVL


( python 3.5 ; tkinter)

##################### GENERAL ############################
import time ; import random
from tkinter import *
window = Tk(); window.title("Conway's Game of Life")
canvas = Canvas(window, width=1200, height=900,bd=0, highlightthickness=0);canvas.pack() 
text_ = Text(window, height=3, width=30); text_.pack() 
text_.insert(END, "To MOVE use arrow keys. Press x to seed. r for random seeds.  f to finish")
##################### VARIABLES ###########################
keypressed = ""; SQident = 1; doublelist=[]    
Neighb=([-50,-50,-50,-50],[0,-50,0,-50],[+50,-50,+50,-50],[-50,0,-50,0],[+50,0,+50,0],[-50,+50,-50,+50],[0,+50,0,+50],[+50,+50,+50,+50])
moves={"Up":(0, -50),"Down":(0,50), "Left":(-50,0), "Right":(50,0)} 
sq = canvas.create_rectangle(50, 50, 100, 100, fill="grey")
##################### CLASS/DEF ###########################
def movesquare(event): # detect movement keys
    x,y=moves[event.keysym]
    canvas.move(SQident,x,y)         
def otherkey(event): #detect other keys pressed
    global keypressed 
    keypressed = event.keysym
    if keypressed == "x": # make new square
        Square(canvas.coords(SQident),"green")

class Square:#Sqa
    def __init__(self,x1y1x2y2,color):
        self.x1y1x2y2=x1y1x2y2
        self.color=color
        if (self.x1y1x2y2) not in doublelist:#create only if there is not one already
            canvas.create_rectangle(self.x1y1x2y2, fill = self.color)
            doublelist.append(self.x1y1x2y2)            
class Neighbours: #Nb
    def __init__(self,squarecoordinates):
        self.squarecoordinates=squarecoordinates
    def numberofneighbours(self):#CN
        return len(canvas.find_overlapping(self.squarecoordinates[0],self.squarecoordinates[1],self.squarecoordinates[2],self.squarecoordinates[3]))-1 # Number of objects surrounding
#################### Bind Keys ###########################        
bindings = {"<KeyPress-Up>" : movesquare, "<KeyPress-Down>": movesquare,
            "<KeyPress-Left>": movesquare, "<KeyPress-Right>": movesquare,
            "x": otherkey, "f": otherkey, "r": otherkey}
for binding in bindings:
    canvas.bind_all(binding, bindings[binding])    
#################### Create field #########################    
while 1:
    window.update();window.update_idletasks()
    if keypressed == "f": #finished
        canvas.delete(SQident)
        break 
    if keypressed == "r": #create 15 random squares
        canvas.delete(SQident)
        for x in range(15):
            x1=(random.randint(0,13)+random.randint(0,13))*50 # between 0-1200 is width
            y1=(random.randint(0,10)+random.randint(0,10))*50 # between 0-900 is height
            x2=x1+50; y2=y1+50
            Sqa = Square([x1,y1,x2,y2],"yellow");
            for y in range(3):#add 3 squares surrounding the previous 15
                ra=random.randint(0,7)
                x1ne=(Neighb[ra][0]);y1ne=(Neighb[ra][1]);x2ne=(Neighb[ra][2]);y2ne=(Neighb[ra][3])
                Sqa = Square([x1+x1ne,y1+y1ne,x2+x2ne,y2+y2ne],"pink")
        break
################### Start animation #########################
color=0; colsel=["Light Pink","Pale Violet Red","Deep Pink","Medium Violet Red","Orchid","Medium Orchid","Medium Purple","Dark Violet","Blue Violet","Purple"]
while 1:
    window.update();window.update_idletasks()
    livelist = canvas.find_all();createlist=[];killlist=[]
    if color == 10:
        color = 1
    for live in livelist: 
        x1=(canvas.coords(live)[0]); y1=(canvas.coords(live)[1]); x2=(canvas.coords(live)[2]); y2=(canvas.coords(live)[3]) 
        Nb=Neighbours([x1,y1,x2,y2]);CN=Nb.numberofneighbours()
        if (CN < 2) or (CN > 3): #dead too few/many neighbours
            killlist.append (canvas.find_enclosed(x1-10,y1-10,x2+10,y2+10)) 
        for Ne in Neighb:
            Encl=len(canvas.find_enclosed(x1+Ne[0]-10,y1+Ne[1]-10,x2+Ne[2]+10,y2+Ne[3]+10)) # enclosed=0 Neighbour is empty; =1 is present
            if Encl == 0: # Neighbour is empty. Possible new life->
                Coo=([x1+Ne[0],y1+Ne[1],x2+Ne[2],y2+Ne[3]])
                Nb=Neighbours(Coo);CN=Nb.numberofneighbours()+1
                if CN == 3: # has 3 neighbours thus new life
                    if Coo not in createlist: # prevent creating twice
                        createlist.append (Coo)                       
    for create in createlist:    #Start creating
        sq=canvas.create_rectangle(create, fill=(colsel[color]))           
    for kill in killlist:        #Start killling
        canvas.delete(kill)        
    for TI in range(0,4):        #Slow down:
        time.sleep(0.1) 
    color +=1
mainloop()

[sixteenornumber]

Does it read well? I normally look for the if __name__ is "__main__": line to figure out where the program starts. Maybe it was your intent to no put there in there and not a big deal. pip8 specifies 79 characters max per line so I would break up some of those long lists.

Spyder is a great IDE for python that helps with pep8. It comes with Anaconda Python. Once it's installed, dig through the install dir for spyder.exe. In Windows this is ".../Anaconda2/scripts/spyder.exe". in linux it's ".../anaconda2/bin/spyder". Not sure what Mac is.

The two classes look funny to me but take that with a grain of salt. I would have done that as a single class but that's not necessarily the correct way.

[Mekire]

So first I want to make clear, the following is not intended to run.  It is just the kind of thing I want to see stylistically.  You will need to deal with the details of making real code out of this:

def bind_keys(canvas):
    """Bind all appropriate keys to the correct function."""
    bindings = {"<keypress-up>" : movesquare, "<keypress-down>": movesquare,
                "<keypress-left>": movesquare, "<keypress-right>": movesquare,
                "x": otherkey, "f": otherkey, "r": otherkey}
    for binding in bindings:
        canvas.bind_all(binding, bindings[binding])    


def setup_phase(window, canvas):
    done = False
    while not done:
        window.update()
        window.update_idletasks()
        if keypressed == "f": #finished
            cells = on_finish_setup(canvas, sqident)
            done = True
        elif keypressed == "r": #create 15 random squares
            cells = on_random_setup(canvas, sqident)
            done = True
        return cells


def on_finish_setup(canvas, sqident):
    # Your func here.


def on_random_setup(canvas, sqident):
    # Your func here.


def main_phase(cells, window, canvas):
    while 1:
        window.update();window.update_idletasks()
        livelist = canvas.find_all()
        createlist, killlist = [],[] 
        update_create_kill(createlist, kill_list)
        render(createlist, killlist)


def main():
    window = tk(); window.title("conway's game of life")
    canvas = canvas(window, width=1200, height=900, bd=0, highlightthickness=0)
    canvas.pack() 
    cells = setup_phase(window, canvas)
    main_phase(cells, window, canvas)


if __name__ == "__main__":
    main()

Note how every function is very short and does one specific thing.  If any further complicated logic is needed it is then put in its own function as well.  One function, one job.  

Other than that you have bad habits that need to be gotten rid of now.
Most of your issues in turning your program into good modular code will be in removing the globals.
I would honestly use classes but this might be too much for the moment.


Any names defined in the global space that are intended to be constant should be capitalized like:  

CELL_SIZE = 50
MOVES = {"up": (0, -CELL_SIZE), "down": (0, CELL_SIZE),
         "left" : (-CELL_SIZE, 0), "right" : (CELL_SIZE, 0)} 

Global variables are not acceptable.  If it is defined globally it better not change.  Consider the global keyword completely off limits.
No more * imports.  Stop that now.  
Also, stop the semicolons to squeeze multiple statements on one line.
Learn proper comment style.  # comments are for short, either inline or mid-function comments.
For real documentation use docstrings.
A doc string looks like this:

def some_function():
    """
    I am a doc string.
    """
    pass

As the previous user suggested you should really adhere to at least the majority of pep8.
You can even use specific IDEs that give you warnings if you are in violation of standards (though honestly that is a bridge too far for me).

[hanscvanleeuwen]

Hi Mekire/sixteenornumber,

Appreciate your input. It feels like I'm kicking a ball for the first time to learn football and Messi steps up to teach me what (not) to do.
Ok steep learning curve here…
I will
-Consider using Spyder as IDE (notepad++ now)
-Read the PEP8 style guide
-use one function-one job
-never use globals as variables (it just worked upon correcting error meassage)
-stop using semicolons ;)
-use classes (hardest one )
-use docstrings and less #
-no more import * ( in my defence, learned that from the webpage https://docs.python.org/2/library/tkinter.html )

Mekire , thanks for the function suggestions. I’ll work on those and post result here.

HCVL

[hanscvanleeuwen]

Hi, I'm struggling with something basic
In the part where I use the movement keys to "draw" squares"

from tkinter import *
window = Tk()
canvas = Canvas(window, width=1200, height=900,bd=0, highlightthickness=0)
canvas.pack() 
text_ = Text(window, height=3, width=30)
text_.pack() 
text_.insert(END, "To MOVE use arrow keys. Press x to seed. ")

moves={"Up":(0, -50),"Down":(0,50), "Left":(-50,0), "Right":(50,0)} 
sq = canvas.create_rectangle(50, 50, 100, 100, fill="grey")

def movesquare(event):
    x,y=moves[event.keysym]
    canvas.move(1,x,y)         
def otherkey(event):
    keypressed = event.keysym
    if keypressed == "x":
        Square(canvas.coords(1))
class Square:
    def __init__(self,x1y1x2y2):
        self.x1y1x2y2=x1y1x2y2
        canvas.create_rectangle(self.x1y1x2y2)

bindings = {"<KeyPress-Up>" : movesquare, "<KeyPress-Down>": movesquare,
            "<KeyPress-Left>": movesquare, "<KeyPress-Right>": movesquare,
            "x": otherkey}
for binding in bindings:
    canvas.bind_all(binding, bindings[binding])   

mainloop()

The suggestion was to replace lines 24-28 with:

def bind_keys(canvas):
    """Bind all appropriate keys to the correct function."""
    bindings = {"<keypress-up>" : movesquare, "<keypress-down>": movesquare,
                "<keypress-left>": movesquare, "<keypress-right>": movesquare,
                "x": otherkey, "f": otherkey, "r": otherkey}
    for binding in bindings:
        canvas.bind_all(binding, bindings[binding])

How do you actually invoke the bind_key in the main?
If I use bind_keys() , I get Type error: missing 1 required positional argument: 'canvas'

[Mekire]

You need to pass it the canvas object.  Something like

def main():
    window = tk.Tk()
    window.title("conway's game of life")
    canvas = tk.Canvas(window, width=1200, height=900, bd=0, highlightthickness=0)
    canvas.pack()    

    bind_keys(canvas)

    cells = setup_phase(window, canvas)
    main_phase(cells, window, canvas)

[hanscvanleeuwen]

If I do that, I get a weird error message:
"bad event type or keysym "keypress"

Thus "keypress" can't be used in a function definition ??

[Mekire]

That is just a capitalization error.  We were having issues with our forum code tags a week ago where they made everything lowercase.

Make sure your function looks like this:

def bind_keys(canvas):
    """Bind all appropriate keys to the correct function."""
    bindings = {"<KeyPress-Up>" : movesquare, "<KeyPress-Down>": movesquare,
                "<KeyPress-Left>": movesquare, "<KeyPress-Right>": movesquare,
                "x": otherkey}
    for binding in bindings:
        canvas.bind_all(binding, bindings[binding]) 

[Kebap]

We were having issues with our forum code tags a week ago where they made everything lowercase.

Your old post way above should now be shown correctly. For example CELL_SIZE is capitalized again. However it seems like the KeyPress is still in lower case?

[Mekire]

Your old post way above should now be shown correctly. For example CELL_SIZE is capitalized again. However it seems like the KeyPress is still in lower case?

A result of cutting and pasting the previously wrong version in to my editor and editing it. I didn't retype his binding code, I cut and paste it while the casing was still all lowered; very annoying.