10
\$\begingroup\$

If anyone has the patience to take a look at my first python snake game, I'd be very grateful for any feedback. I am fairly new to programming and Python, but am looking to improve so any constructive feedback is appreciated.

import Tkinter
import time
import random
UP = u'\uf700'
DOWN = u'\uf701'
LEFT = u"\uf702"
RIGHT = u'\uf703'
TIMESTEP = 50
WIDTH = 300
HEIGHT = 300
colours = ['blue','red','pink','yellow','cyan','magenta','green']
class game(object):
 def __init__(self):
 self.window = Tkinter.Tk()
 self.canvas = Tkinter.Canvas(self.window, width = WIDTH, height = HEIGHT)
 self.canvas.pack()
 self.leader = Rect(self.canvas,50,50,60,60,'red',10,0)
 self.rect2 = Rect(self.canvas,40,50,50,60,'blue',10,0)
 self.test1 = Rect(self.canvas,100,100,110,110,'pink',0,0)
 self.followers = [self.rect2,self.test1,Rect(self.canvas,200,200,210,210,'yellow',0,0)]
 def moveit(self):
 # moves the leader and the followers follow!
 prevpos = (self.leader.coords()[0],self.leader.coords()[1])
 self.leader.move(self.leader.getvel()[0],self.leader.getvel()[1])
 for rect in self.followers:
 x = (rect.coords()[0],rect.coords()[1])
 self.moveto(rect,prevpos)
 prevpos = x
 self.eatapple()
 self.selfcollision()
 self.throughwalls()
 self.window.after(TIMESTEP,self.moveit)
 def moveto(self,box1,pos):
 #moves box1 to pos
 box1.move(pos[0]-box1.coords()[0],pos[1]-box1.coords()[1])
 def makeapple(self):
 #This randomly puts an apple on the canvas
 x1 = random.randrange(0,WIDTH,10)
 y1 = random.randrange(0,HEIGHT,10)
 self.apple = Rect(self.canvas,x1,y1,x1+10,y1+10, 'purple',0,0)
 def eatapple(self):
 #This check for eating apple, if eaten, creates new apple and grows the snake
 if self.leader.coords() == self.apple.coords():
 self.canvas.delete(self.apple.id)
 self.makeapple()
 self.grow()
 def grow(self):
 # grows the snake by appending a rectangle to the followers list
 x1 = self.followers[-1].coords()[0]
 y1 = self.followers[-1].coords()[1]
 x2 = self.followers[-1].coords()[2]
 y2 = self.followers[-1].coords()[3]
 self.followers.append(Rect(self.canvas,x1,y1,x2,y2,colours[random.randint(0,len(colours)-1)],0,0))
 def selfcollision(self):
 # This will check for collision with itself
 for follower in self.followers:
 if self.leader.coords() == follower.coords():
 print "You lose!"
 self.window.destroy()
 def throughwalls(self):
 # This will put the snake on the klein bottle geometry!
 x1,y1,x2,y2 = self.leader.coords()
 if x1>=WIDTH:
 self.moveto(self.leader,(0,HEIGHT-y1))
 if x1<0:
 self.moveto(self.leader,(WIDTH,HEIGHT-y1))
 if y1>= HEIGHT:
 self.moveto(self.leader,(x1,0))
 if y1< 0:
 self.moveto(self.leader,(x1,HEIGHT))
 def keyup(self,event):
 if self.leader.velocity != (0,10):
 self.leader.velocity=(0,-10)
 #print "UP!"
 def keydown(self,event):
 if self.leader.velocity != (0,-10):
 self.leader.velocity = (0,10)
 #print "DOWN!"
 def keyright(self,event):
 if self.leader.velocity != (-10,0):
 self.leader.velocity = (10,0)
 def keyleft(self,event):
 if self.leader.velocity != (10,0):
 self.leader.velocity = (-10,0)
 def mainloop(self):
 self.window.bind("<Key-Up>",self.keyup)
 self.window.bind("<Key-Down>",self.keydown)
 self.window.bind("<Key-Left>",self.keyleft)
 self.window.bind("<Key-Right>",self.keyright)
 self.makeapple()
 self.moveit()
 self.window.mainloop()
class Rect(object):
 def __init__(self,canvas,x1,y1,x2,y2,color,vx,vy):
 self.canvas = canvas
 self.velocity = vx,vy
 self.id = self.canvas.create_rectangle(x1,y1,x2,y2,fill = color)
 def getvel(self):
 return self.velocity
 def coords(self):
 return self.canvas.coords(self.id)
 #self.root = Tkin
 def move(self,vx,vy):
 self.canvas.move(self.id,vx,vy)
canvas = game()
canvas.mainloop()
while True:
 ans=raw_input("Another game? (y/n) ")
 if ans== 'n':
 break
 elif ans == 'y':
 canvas = game()
 canvas.mainloop()
 else:
 print "What was that?"

Also, this snake lives on the Klein bottle, as opposed to the usual Torus snake.

One specific question:

Games such as this require a loop that moves the blocks around the canvas. One can achieve this with a while loop, however I read somewhere that this was not advised as there is already a mainloop() running. Instead, one should use Tkinter's "after" function. Can anyone explain why this is favourable?

200_success
145k22 gold badges190 silver badges478 bronze badges
asked Aug 25, 2015 at 20:38
\$\endgroup\$
7
  • 1
    \$\begingroup\$ Does this currently run for you? I am not able to run it as is and am encountering several errors. \$\endgroup\$ Commented Aug 25, 2015 at 20:56
  • \$\begingroup\$ @benalbrecht Sorry. No it doesnt run for me either. I think copying it into the question has introduced some formatting errors. It there are better way to enter code than just copy and past? \$\endgroup\$ Commented Aug 25, 2015 at 21:01
  • \$\begingroup\$ Or perhaps I should ask, how does one correctly paste code into a SE question? \$\endgroup\$ Commented Aug 25, 2015 at 21:05
  • \$\begingroup\$ One easy way: paste the code, then select (highlight) the code using the mouse, and press Control-K : you will notice it shifts all the selected text to the right by 4 spaces \$\endgroup\$ Commented Aug 25, 2015 at 21:07
  • \$\begingroup\$ Got it, thanks. But I still seem to have a problem running the code... \$\endgroup\$ Commented Aug 25, 2015 at 21:10

1 Answer 1

9
\$\begingroup\$

Coding style

Please follow PEP8, the style guide of Python. The code will become so much easier to read.

More tuple-assignments please

Use tuple-assignments more aggressively. For example, instead of this:

 x1 = self.followers[-1].coords()[0]
 y1 = self.followers[-1].coords()[1]
 x2 = self.followers[-1].coords()[2]
 y2 = self.followers[-1].coords()[3]

This is simpler:

 x1, y1, x2, y2 = self.followers[-1].coords()

Avoid repeated calls

Use tuple/list slicing more aggressively. For example, these expressions are long and tedious:

prevpos = (self.leader.coords()[0],self.leader.coords()[1])
self.leader.move(self.leader.getvel()[0],self.leader.getvel()[1])

This is simpler:

prevpos = self.leader.coords()[:2]
self.leader.move(*self.leader.getvel()[:2])

Not only this is simpler, but it avoids making repeated method calls.

Use elif

These conditions are mutually exclusive:

 if x1>=WIDTH:
 self.moveto(self.leader,(0,HEIGHT-y1))
 if x1<0:
 self.moveto(self.leader,(WIDTH,HEIGHT-y1))

As they cannot happen at the same time, you should chain them with an elif.

 if x1>=WIDTH:
 self.moveto(self.leader,(0,HEIGHT-y1))
 elif x1<0:
 self.moveto(self.leader,(WIDTH,HEIGHT-y1))

Strange API

Most of the functions of game are not natural:

  • moveto
  • eatapple
  • grow
  • selfcollision
  • throughwalls

A game that moves? And eats apple?

Methods should represent actions that can be performed on the class. Since "selfcollision" and "throughwalls" are not verbs, it's not trivial what they represent. All these methods make for a very strange API.

answered Aug 25, 2015 at 21:24
\$\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.