• Advertisement
Sign in to follow this  

Python/pygame Code Review - Where To Next ?

This topic is 640 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi All.


I've been working on a small python game for a BBC micro:bit which is an interesting little device targeted at schools.

As a side project I started working on an expanded version that uses pygame.  The idea was to build a framework that could be used to make simple games.  There a no proper graphics yet.  All items are basically rectangles for now.


It's almost there and I was wondering if someone would mind looking over the code and recommending any improvements or perhaps a new direction I should take the project.


One obvious problem is I haven't used any python classes etc.  Everything is held in lists.  I found with the micro:bit, which has very little memory, that classes/instances use a lot more memory than a simple list.  No real excuse why the PC version couldn't be converted to classes ;-)


Should I simply post the code here? or maybe port on another site and link ? It's about 170 lines.


Any advice would be great.

Share this post

Link to post
Share on other sites

 Ok here it is.. 


setup for a 800x800 windows so change size & onScreen if you want bigger smaller.


control "tank" with arrow keys and fire with x.

import random,pygame,time, math
from pygame.locals import *

def countdown(item):
	if item[5]<0:item[5]=0
	if item[9]<0:item[9]=0
def offscreen(item):
	if item[1]<0 or item[1]>(size-item[7]) or item[2]<0 or item[2]>(size-item[8]):
		return 1

def turn(item):
def doCollision(no,item,type): #[j,wave[j],processor[1]
	if(type==1):		# item stop
	elif(type==2):	# item explode
	elif(type==3):	# item reverse
	elif(type==4):	# wrap around screen
		if item[1]<0:item[1]=(size-item[7])
		elif item[1]>(size-item[7]):item[1]=0
		elif item[2]<0:item[2]=(size-item[8])
		elif item[2]>(size-item[7]):item[2]=0
	elif(type==5):	# item turn

def makeEnemy(delay):
	c[not b]=random.randint(0,1)*sizetmp
	if x==0:v.append([1,0])
	elif x==sizetmp:v.append([-1,0])
	if y==0:v.append([0 ,1])
	elif y==sizetmp:v.append([0,-1])
	return [11,x,y,v[d][0],v[d][1],0,s,side,side,delay,x,y]	# 0-type,1-x,2-y,3-vx,4-vy,5-life in ms,6-speed,7-sizex, 8-sizey, 9-collsion delay, 10-last safe x, 11- last safe y

wave.append([1,size/2,size/2,0,-1,9999,2,25,25,25,100,50,50])			#player
wave.append([15,100,80,0,0,1000,0,100,50,0,0,0]) #wall
wave.append([15,50,180,0,0,9999,0,50,100,0,0,0])	# 11- enemy, 12- shell, 13- explosion, 15- wall.
processCollision={(15,1):(0,1),(1,15):(1,0),(11,15):(5,0),(15,11):(0,5),(12,15):(2,0),(15,12):(0,2),(1,99):(4,0),(13,99):(0,0),(11,99):(4,0),(12,99):(2,0)}	 # 0 - nothing, 1 - stop, 2 - explode, 3 - reverse, 4 - wrap
for j in range(no_enemies):wave.append(makeEnemy(200))
while 1:
	for i in range(len(wave)-1,-1,-1):
		typ=wave[i][0]	# what is item type ?
		countdown(wave[i]) # reduce life and collision delay timers if they above 0.
		if typ==12 or typ==13:	# only shells or explosions have a set lifespan
			if not wave[i][5]:
				del wave[i]
		if typ==11:	# enemy randomly fire in random directions
			if random.randint(0,500)>497:
			if random.randint(0,500)>498: #and randomly turns
		if typ==13:	# explosions expand
		if typ==1:		# ie player
			pressed = pygame.key.get_pressed()
			if pressed[pygame.K_RIGHT]:v[0]=1
			if pressed[pygame.K_LEFT]:v[0]=-1
			if pressed[pygame.K_UP]:v[1]=-1
			if pressed[pygame.K_DOWN]:v[1]=1
			if v[0] or v[1]:	
			if pressed[pygame.K_x]: 
				if reloading<0:
			wave[i][1]+=int(wave[i][6]*wave[i][3]*dtime)  # move x	i.e 50 pixels per second. * direction -1 * 0.017
			wave[i][2]+=int(wave[i][6]*wave[i][4]*dtime)  # move y
		if random.randint(0,1000)>999:
		if not wave[i][9]:
			if typ==13:
				pygame.draw.ellipse(screen,(255,0,0), (wave[i][1],wave[i][2],wave[i][7],wave[i][8]),1)
				if typ==1:		# ie player
	for i in range(len(wave)-1,-1,-1):
		if offscreen(wave[i]):
			processor=processCollision.get((wave[i][0],99))## doCollision(i,wave[i],processor)
		else:		# if item is not offscreen then check for collsions against everything else.
			for j in range(i-1,-1,-1): # player position 0 will only ever feature in j
				if wave[j][9] or wave[i][9]:continue # if either item is still in a collision delay skip.
				if (wave[i][0],wave[j][0]) in ignoreCollision:	# we are not interested in some collisions.ie explosions with explosions  Don't bother with any further processing
				if wave[j][1]<wave[i][1]+wave[i][7]:#1x<2x+2s
					if wave[j][1]+wave[j][7]>wave[i][1]:#1x+1s>2x
						if wave[j][2]<wave[i][2]+wave[i][8]:#1y<2y+2s
							if wave[j][2]+wave[j][8]>wave[i][2]:#1y+1s>2y
								if (wave[i][0],wave[j][0]) in processCollision:	# if we dont have a processor for this combination of items colliding kill both of them
									break	# doesn't consider 3 way
	for k in reversed(sorted(kill.keys())):
		del wave[k]
	main_surface.blit(screen, (0,0))

Share this post

Link to post
Share on other sites

I find it quite difficult to understand what's going on in your code.


A part from using objects or dictionaries to store your data instead of lists, which you already pointed, I recommend to give meaningful names to your variables. For instance, in your method turn I have no idea what are v or u supposed to be.


These changes should improve a lot your code readability, which I think is the biggest issue right now. Once this is fixed you'll probably be able to get better suggestions for improvement.

Share this post

Link to post
Share on other sites

a) Add more comments. You should have comments explaining what every section of code does, as well as your existing comments about certain lines of code. Docstrings for functions are a good idea too.


b) Don't use comments to explain magic numbers - replace the numbers with named constants. eg. ObjType_Enemy = 11, ObjType_Shell = 12. This is especially essential if you don't want to use classes and just want to use parts of a list. I'm not going to spend time working out what item 7 or item 11 of an object are.


c) Importing '*' from anywhere is usually a bad idea. It makes it hard to see what is a global you've defined and what has been pulled in from some other module.


d) Put your main logic into a function, instead of having it at module level. This stops you from getting a ton of unnecessary global variables.


e) Don't create the screen object fresh every frame. (Any reason you can't just draw to the main surface?)


f) Don't do "for i in range(len(whatever))". This is a bad habit that people pick up when they try to make Python act like C++ or Basic, because they are used to having a loop index, but then they typically convert that right back to the object they would have got from a normal iteration. In this case, "for i in range(len(wave)-1,-1,-1)" is followed by a ton of "wave" references - so why not just have "for current_wave in reversed(wave)"? By making this change to your two loops, you can get rid of most of that ugly array indexing.


g) Don't have if-state?ments and loops 9 levels deep. Factor some of that out into functions


h) Why reverse and sor?t a list of keys if all you're doing is deleting each one from the wave list? They're all getting deleted, so the order doesn't matter.

Share this post

Link to post
Share on other sites

Hi - I'll put together a commented version as looking at it objectively I can see your point.  It's not easy to see what variables are used for.  Sorry about that.


The turn function basically updates the vector for the given item so it turns 90 degrees.  I've also just spotted a problem with the original so I've updated it here....

def turn(item):
	v=[]					# empty list ( temp / local )
	v.append([item[4],-item[3]])		# add one alternate vector.  use y,-x
	v.append([-item[4],item[3]])	# add another alternate vector ( list of lists ). use -y,x
	u=random.randint(0,len(v)-1)		# pick one at random
	item[3]=v[u][0]				# update the item x vector
	item[4]=v[u][1]				# update the item y vector
	item[1]=item[10]			# rewind the item's x position to it's last known good x value ( pre collision )
	item[2]=item[11]			# do the same  with y

All game items are held in wave[] which is a list of lists.  All game items have the same attributes;


0-item type ( 1 - player, 11 - enemy, 12 - shell, 13 - explosion, 15 - wall, 99 - edge of screen )

1-x position

2-y position

3-vx - vector x

4-vy - vector y

5-life in ms.  used to countdown the life span of certain game items ( shell, explosions ).  Limits life and/or range

6-speed in px/second

7-size x

8-size y

9-collision delay.  if >0 item is NOT included in collision detection.

10-last safe x position - used to 'rewind' item position during a collision.

11-last safe y position - ditto



so wave.append([13,50,60,0,0,500,0,50,50,0,0,0]) would create an explosion at 50,60 which would last for 0.5 seconds.  I know classes would be ALOT better. :)

Share this post

Link to post
Share on other sites

Hi Kylotan               


Thanks for the pointers.  I''ll be looking at a rewrite as I think that would be easier than re-hashing what I have.  Alot of great advice.  Getting rid of the endless wave statements would also be a great idea so thanks again for that.  Splitting into function will make things a lot easier to read.


The backward list thing is a hangover from when I was deleting elements out of wave[] while iterating over wave[].  You are 100% right as I don't need to do this any more as the deletes are handled later on with the kill dict.


So do you not think I don't need to do the...

for k in reversed(sorted(kill.keys())):	
	del wave[k]

I was thinking I still need to delete items out of wave in reverse but maybe not ? i'll give it a go in the rewrite.




Share this post

Link to post
Share on other sites

I see what you're saying about the order. But a better way to remove a set of items from a list is to just replace it with a new list that lacks the items you want to remove. eg.


wave = [x for x in wave if x not in kill]

Edited by Kylotan

Share this post

Link to post
Share on other sites

I'm sure you have the indices memorized right now, but, if you stop working on this for a bit, then come back later, will you remember what 6 and 7 and 10 are?  Probably not, so, you still need to replace these magic numbers with constants.  Something as simple as this:

def turn(item):
    v=[]                    # empty list ( temp / local )
    v.append([item[VY],-item[VX]])      # add alternate vectors. use y,-x and -y,x
    u=random.randint(0,len(v)-1)        # pick one at random
    item[VX]=v[u][1]                    # update the x and y vector
    item[X]=item[SAFE_X]                # rewind item's x and y position to last know good value (pre collision)

Also, you don't need to comment every line.  Only comment where it makes sense.

For example, it's obvious, if you rewind x, you're doing the same thing for y.  So you could say rewind item's x and y position to last know good value (pre collision)

Edited by BeerNutts

Share this post

Link to post
Share on other sites
Sign in to follow this  

  • Advertisement