Public Group

# Python/pygame Code Review - Where To Next ?

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

## 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.

##### Share on other sites

Post the code here. Ideally in-line, if possible (so nobody has to download a zip and extract it, etc).

##### 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):
item[5]-=time_passed
item[9]-=time_passed
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):
v=[]
v.append([item[4],item[3]])
v.append([item[4]*-1,item[3]*-1])
u=random.randint(0,len(v)-1)
item[3]=v[u][0]
item[4]=v[u][1]
item[1]=item[10]
item[2]=item[11]

def doCollision(no,item,type): #[j,wave[j],processor[1]
if(type==1):		# item stop
item[1]=item[10]
item[2]=item[11]
item[3]=0
item[4]=0
elif(type==2):	# item explode
kill[no]=1
wave.append([13,item[1],item[2],0,0,400,3,item[7],item[8],0,0,0])
elif(type==3):	# item reverse
item[3]*=-1
item[4]*=-1
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
turn(item)

def makeEnemy(delay):
c=[0,0]
b=random.randint(0,1)
sizetmp=size-20
c[b]=random.randint(0,sizetmp)
c[not b]=random.randint(0,1)*sizetmp
x=c[0]
y=c[1]
v=[]
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])
d=random.randint(0,len(v)-1)
s=random.randint(60,70)
side=random.randint(6,25)
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

no_enemies=20
size=800
onScreen=800
wave=[]
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.
wave.append([15,300,150,0,0,9999,0,50,200,0,0,0])
wave.append([15,100,330,0,0,9999,0,170,50,0,0,0])
ignoreCollision={(13,13):1,(13,12):1,(12,13):1,(13,15):1,(15,13):1,(15,15):1}
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))
pygame.init()
main_surface=pygame.display.set_mode((onScreen,onScreen))
clock=pygame.time.Clock()
while 1:
time_passed=clock.tick(60)
dtime=time_passed/1000
screen=pygame.Surface((size,size))
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.
m=int(wave[i][7]/2)
if typ==12 or typ==13:	# only shells or explosions have a set lifespan
if not wave[i][5]:
del wave[i]
continue
if typ==11:	# enemy randomly fire in random directions
if random.randint(0,500)>497:
wave.append([12,wave[i][1]+m,wave[i][2]+m,random.randint(-1,1),random.randint(-1,1),2000,150,3,3,200,0,0])
if random.randint(0,500)>498: #and randomly turns
turn(wave[i])
if typ==13:	# explosions expand
wave[i][7]+=1*60*dtime
wave[i][8]+=1*60*dtime
wave[i][1]-=1*30*dtime
wave[i][2]-=1*30*dtime
if typ==1:		# ie player
pressed = pygame.key.get_pressed()
v=[0,0]
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]:
wave[i][3]=v[0]
wave[i][4]=v[1]
wave[i][10]=wave[i][1]
wave[i][11]=wave[i][2]
wave[i][1]+=int(v[0]*dtime*150)
wave[i][2]+=int(v[1]*dtime*150)
if pressed[pygame.K_x]:
wave.append([12,wave[i][1]+m,wave[i][2]+m,wave[i][3],wave[i][4],1000,300,3,3,100,0,0])
else:
wave[i][10]=wave[i][1]
wave[i][11]=wave[i][2]
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:
wave.append(makeEnemy(200))
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)
else:
pygame.draw.rect(screen,(255,255,255),(wave[i][1],wave[i][2],wave[i][7],wave[i][8]),1)
if typ==1:		# ie player
pygame.draw.line(screen,(255,255,255),(wave[i][1]+m,wave[i][2]+m),(wave[i][1]+m+(wave[i][3]*17),wave[i][2]+m+(wave[i][4]*17)))
kill={}
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)
doCollision(i,wave[i],processor[0])
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
continue
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
processor=processCollision.get((wave[i][0],wave[j][0]))
doCollision(i,wave[i],processor[0])
doCollision(j,wave[j],processor[1])
else:
kill[i]=1
kill[j]=1
wave.append([13,wave[j][1],wave[j][2],0,0,1000,3,wave[j][7],wave[j][8],0,0,0])
break	# doesn't consider 3 way
for k in reversed(sorted(kill.keys())):
del wave[k]
screen=pygame.transform.scale(screen,(onScreen,onScreen))
main_surface.blit(screen, (0,0))
pygame.display.flip()
pygame.event.pump()


##### 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 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 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 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 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 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
v.append([-item[VY],item[VX]])
u=random.randint(0,len(v)-1)        # pick one at random
item[VX]=v[u][1]                    # update the x and y vector
item[VY]=v[u][2]
item[X]=item[SAFE_X]                # rewind item's x and y position to last know good value (pre collision)
item[Y]=item[SAFE_Y]


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

1. 1
2. 2
3. 3
4. 4
Rutin
12
5. 5

• 12
• 17
• 10
• 14
• 10
• ### Forum Statistics

• Total Topics
632660
• Total Posts
3007694
• ### Who's Online (See full list)

There are no registered users currently online

×