2
\$\begingroup\$

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()
\$\endgroup\$
1
\$\begingroup\$

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
\$\endgroup\$
  • \$\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 Jul 1 '18 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 Jul 1 '18 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 Jul 1 '18 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 Jul 1 '18 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 Jul 1 '18 at 18:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.