This is a very simple ball game in which we have to avoid projectiles falling from the sky. This is my second project and first game. Please review and tell where I can improve.
import time
import random
import pygame
pygame.init()
#colors
black=(0,0,0)
white=(255,255,255)
red=(255,0,0)
green=(0,255,0)
blue=(0,0,255)
darkViolet=(148,0,211)
#display
displayWidth=1024
displayHeight=600
display=pygame.display.set_mode((displayWidth, displayHeight))
pygame.display.set_caption('PHY')
heart=pygame.image.load('heart.png')
myfont=pygame.font.SysFont('Terminal', 18)
clock=pygame.time.Clock()
endTime=time.time()
#physics
x,y=displayWidth/2,displayHeight/2
ux, uy, ax, ay= 0, 0, 0, 0 # u means velocity, a means acceleration
yp1, up1, ap = 0, 50, 50 #p for projectile
xp1,yp1,up1,ap1=[],0,50,50 #projectile variables. xp1 list coz multiple projectiles
uc, ac = 20, 5 # c stands for the amount of change on key press
score, lives=0,3
touching=False
running=True
#misc
projectile=pygame.Surface((10,20))
projectile.fill(darkViolet)
#adding random locations for projectiles to spawn in
for _ in range(20):
xp1.append(random.randint(0,displayWidth))
while running:
#taking dt to be a small value which will depend on the processing power
startTime=time.time()
t=startTime-endTime
#changing the postions and velocities with time
ux+=ax*t
uy+=ay*t
x+=ux*t
y+=uy*t
up1+=ap*t
yp1+=up1*t
endTime=time.time()
#checking for collision of ball with boundaries
if x<0:
x=0
ux=-ux/3
if x>displayWidth:
x=displayWidth
ux=-ux/3
if y<0:
y=0
uy=-uy/3
if y>displayHeight:
y=displayHeight
uy=-uy/3
for ev in pygame.event.get():
if ev.type == pygame.QUIT:
running=False
elif ev.type == pygame.KEYDOWN: #acts on pressing and not on holding key
if ev.key == pygame.K_UP:
ay-=ac
uy-=uc
if ev.key == pygame.K_DOWN:
ay+=ac
uy+=uc
if ev.key == pygame.K_LEFT:
ax-=ac
ux-=uc
if ev.key == pygame.K_RIGHT:
ax+=ac
ux+=uc
elif ev.type == pygame.KEYUP:
if ev.key == pygame.K_UP:
ay=0
if ev.key == pygame.K_DOWN:
ay=0
if ev.key == pygame.K_LEFT:
ax=0
if ev.key == pygame.K_RIGHT:
ax=0
#condition for when the projectile crosses the screen
if yp1>displayHeight:
yp1=0
up1=3*up1/5
ap+=5
xp1=[]
score+=1
for _ in range(20):
xp1.append(random.randint(0,displayWidth))
#checking for collision between ball and projectile
for g in range(20):
if x>xp1[g]-10 and x<10+xp1[g] and y>yp1-15 and y<yp1+15:
touching=True
xp1[g]=1050
if touching:
if lives>1:
lives-=1
touching=False
else:
running=False
#displaying
display.fill(black)
for g in range(lives): #displaying the lives as hearts
display.blit(heart, (950+g*25,25))
for g in range(20): #displaying the same projectile at 20 places
display.blit(projectile, (xp1[g], yp1))
textDisp=myfont.render('SCORE: %s'%(score),False,white)
pygame.draw.circle(display, red, (int(x),int(y)), 10, 0) #displaying the ball
display.blit(textDisp,(950,50)) #displaying the score
pygame.display.update()
clock.tick(60)
pygame.quit()
quit()
1 Answer 1
Follow standard identifier conventions. Variable are lower_case
, class names should be CamelCase
, and constants should be UPPER_CASE
. With this in mind, your block of colours at the start should be altered:
# Colours
BLACK = (0, 0, 0)
WHITE = (255, 255, 255)
RED = (255, 0, 0)
#... etc ...
Named colours are fine, but what are you using them for? Maybe, after naming colours, name some of the usages, and assign the named colours.
BALL_COLOUR = RED
SCORE_COLOUR = WHITE
Note the spacing above.
ux+=ax*t
uy+=ay*t
x+=ux*t
y+=uy*t
... I find hard to read. Add a space before and after operators, a space after commas, etc.
ux += ax * t
uy += ay * t
x += ux * t
y += uy * t
... is much easier to read.
score, lives=0,3
Tuple unpacking assignment is a wonderful feature. But don’t use it for unrelated values, like score and lives. Use two separate assignment statements. But when you do use it, remember spaces around the equals, and after the commas.
Declare variables as close as possible to where you use them. What is this running = True
statement, near the start? Is there someone walking and running as well as bouncing a ball?
running = True
while running:
# ...
This is much clearer. The value controls the loop.
Watch for "if condition, do something, else if different condition, do same thing". Use an "or
" in the test.
if ev.key == pygame.K_UP or ev.key == pygame.K_DOWN:
ay = 0
Don’t intermix unrelated code.
textDisp = ...
pygame.draw.circle(...)
display.blit(textDisp, ...)
Keep the two textDisp
lines together; the circle draw code is unrelated and should be moved before or after this code.
Elephant in the room: Variable names. They are terrible. I’d suggest better ones, but I’d suggest using classes for player/projectile even more. And maybe a function or two for organization. But as this is your third project, I don’t want to confuse you with constructs you may not have learned yet.
Let us know what you know, and perhaps we can help further.
Inefficiency: repeating the same test 20 times. Both y and yp1 remain constant in this loop:
for g in range(20):
if x>xp1[g]-10 and x<10+xp1[g] and y>yp1-15 and y<yp1+15:
Instead, check for a y-collision first, and only if it is a possibility, then check the 20 x-collisions.
if y > yp1 - 15 and y < yp1 + 15:
# loop over all xp1 values
Hard-coded numbers. You repeat 20 multiple times throughout the code. What if you want to change that? Find every place it is used and change it? Better: use a variable for num_projectiles. Even better, just iterate over the storage structure itself:
if y > yp1 - 15 and y < yp1 + 15:
for xp in xp1:
if x > xp - 10 and x < xp + 10:
# collision code
-
\$\begingroup\$ Thanks for the reply. I did everything you said. I also thought about using classes and I also tried but it was only making the code longer than it is and also I was encoutering a lot of problems so I stopped. One thing that needs improvement here is the detection of collision between ball and projectile. It doesn't work sometimes. Do you have any suggestion for that? \$\endgroup\$hjjinx– hjjinx2018年07月01日 06:32:49 +00:00Commented Jul 1, 2018 at 6:32
-
\$\begingroup\$ Longer code is not itself an issue. Proper organization is required, though. Getting it working, and getting in the habit of using proper structures is worth the effort. \$\endgroup\$AJNeufeld– AJNeufeld2018年07月01日 17:46:01 +00:00Commented Jul 1, 2018 at 17:46
-
\$\begingroup\$ Code Review is for reviewing working code, to improve structure, efficiency, coding practices ... and maybe spotting previous unknown/undetected bugs. It is not for debugging code. If your collision detection isn’t working, then your code shouldn’t be posted here. Stack Overflow would be more appropriate. \$\endgroup\$AJNeufeld– AJNeufeld2018年07月01日 17:54:46 +00:00Commented Jul 1, 2018 at 17:54
-
\$\begingroup\$ Oh ok. Thanks for all your time I applied everything except for the part you suggested for changing in #collision between ball and projectile which can't be done so because I also have to change the value of xp[g] to make the projectile disappear. \$\endgroup\$hjjinx– hjjinx2018年07月01日 18:00:03 +00:00Commented Jul 1, 2018 at 18:00
-
\$\begingroup\$ Which is another ugly hard-coded number issue. 1050? What if you later expand your game width to 1200? \$\endgroup\$AJNeufeld– AJNeufeld2018年07月01日 18:07:28 +00:00Commented Jul 1, 2018 at 18:07