Can someone review the code for this dodging game I made a while back? I want to see if there are any improvements or optimizations I can make as I am more or less a Python beginner.
# A basic dodging game in Pygame
import pygame, sys
import numpy as np
from pathlib import Path
pygame.init()
# Defines window size
win_size = (900, 600)
window = pygame.display.set_mode(win_size)
pygame.display.set_caption("Dodge")
clock = pygame.time.Clock()
scores = np.loadtxt(str(Path().absolute()) + "/scores.txt").astype(np.int64)
try:
high_score = scores.max()
except ValueError:
high_score = 0
running = True
timer = 0
projectile_speed = 7 # Defines projectile speed
score = 0
score_font = pygame.font.SysFont("dejavusansmono", 24)
game_over_font = pygame.font.SysFont("dejavusansmono", 48)
title_text = score_font.render("Score: ", True, (45, 45, 45))
score_text = score_font.render(str(score), True, (110, 110, 110))
# AABB Collision implementation
def aabb(r1, r2):
return r1.x < r2.x + r2.width and r1.x + r1.width > r2.x and r1.y < r2.y + r2.height and r1.y + r1.height > r2.y
# Displays game over content if input(collision) is true
def game_over(val):
if val:
game_over_text = game_over_font.render("GAME OVER!", True, (0, 0, 0))
window.blit(game_over_text, (win_size[0]/2 - game_over_text.get_width()/2, win_size[1]/2 - game_over_text.get_height()/2))
pygame.display.update()
pygame.time.wait(1000)
if score > high_score:
title_text = score_font.render("New Highscore!", True, (45, 45, 45))
window.blit(title_text, (win_size[0]/2 - game_over_text.get_width()/2, win_size[1]/2 + game_over_text.get_height()/2))
else:
title_text = score_font.render("Final Score: ", True, (45, 45, 45))
score_text = score_font.render(str(score), True, (110, 110, 110))
window.blit(title_text, (win_size[0]/2 - game_over_text.get_width()/2, win_size[1]/2 + game_over_text.get_height()/2))
window.blit(score_text, (win_size[0]/2 - game_over_text.get_width()/2 + title_text.get_width(), win_size[1]/2 + game_over_text.get_height()/2))
pygame.display.update()
pygame.time.wait(1000)
return False
return True
# Object class that parents Player and Projectile classes
class Object:
global objects
objects = []
def __init__(self, x, y, w, h):
self.x = x
self.y = y
self.width = w
self.height = h
objects.append(self)
def draw(self):
pygame.draw.rect(window, "red", (self.x, self.y, self.width, self.height)) # Default draw method
# Defines Projectile class (child of Object)
class Projectile(Object):
# Global projectile list stores all projectile data
global projectiles
projectiles = []
def __init__(self, x, y, w, h, speed):
super().__init__(x, y, w, h)
self.dir = -1 * (x/abs(x)) # Calucates required sprite direction based on input value
self.speed = speed * self.dir
projectiles.append(self)
def update(self):
self.x += self.speed
@classmethod
def update_all(self):
valid_projectiles = [projectile for projectile in projectiles if (projectile.x > 0 - projectile.width and projectile.dir == -1) or (projectile.x < win_size[0] and projectile.dir == 1)] # Projectiles within bounds
invalid_projectiles = [projectiles.pop(projectiles.index(projectile)) for projectile in projectiles if (projectile.x < 0 - projectile.width and projectile.dir == -1) or (projectile.x > win_size[0] and projectile.dir == 1)] # Projectiles NOT within bounds
del invalid_projectiles
# Running all projectile methods
for projectile in valid_projectiles:
projectile.update()
projectile.draw()
# Defines Player class (child of Object)
class Player(Object):
def __init__(self, x, y, w, h, speed):
super().__init__(x, y, w, h)
self.speed = speed
self.touched = False
def draw(self):
pygame.draw.rect(window, "blue", (self.x, self.y, self.width, self.height)) # Player draw method
def update(self):
self.keys = pygame.key.get_pressed()
self.x += self.speed * (self.keys[pygame.K_RIGHT] - self.keys[pygame.K_LEFT]) # Left/Right Key Input
self.y += self.speed * (self.keys[pygame.K_DOWN] - self.keys[pygame.K_UP]) # Down/Up Key Input
def set_bounds(self):
# Restricts player to screen dimensions
if self.x < 0:
self.x = 0
elif self.x > win_size[0] - self.width:
self.x = win_size[0] - self.width
if self.y < 0:
self.y = 0
elif self.y > win_size[1] - self.height:
self.y = win_size[1] - self.height
self.yvel = 0
def collision(self):
# Defines collisions w/ Projectiles
for projectile in projectiles:
if aabb(player, projectile):
return True
return False
def update_all(self):
# Running Player methods
self.update()
self.set_bounds()
self.draw()
player = Player(win_size[0]/2 - 16, win_size[1]/2 - 16, 32, 32, 5) # Defines player
while running: # Game loop
window.fill("white") # Clears screen
timer += 1
if timer % 10 == 0: # New Projectile object every 10 frames
Projectile(np.random.choice((-32, win_size[0])), np.random.randint(0, win_size[1] - 32), 32, 16, projectile_speed - np.random.random())
if timer % 60 == 0: # Score increments by 1 every 60 frames
score += 1
projectile_speed += 0.2 # Increases every second: very basic difficulty scaling
if projectile_speed > 14: # Caps off bullet speed
projectile_speed = 14
player.update_all() # Updates Player methods
Projectile.update_all() # Updates Projectile methods
for event in pygame.event.get():
if event.type == pygame.QUIT:
running = False
pygame.quit()
sys.exit()
title_text = score_font.render("Score: ", True, (45, 45, 45))
score_text = score_font.render(str(score), True, (110, 110, 110))
window.blit(title_text, (0, 0))
window.blit(score_text, (title_text.get_width(), 0))
title_text = score_font.render("Highscore: ", True, (45, 45, 45))
score_text = score_font.render(str(high_score), True, (110, 110, 110))
window.blit(title_text, (0, title_text.get_height()))
window.blit(score_text, (title_text.get_width(), title_text.get_height()))
title_text = score_font.render(str(np.int64(clock.get_fps())), True, (45, 45, 45))
window.blit(title_text, (0, win_size[1] - title_text.get_height()))
running = game_over(player.collision())
# Constantly calls game_over function
# Returns false when player.collision() is true
clock.tick(60)
pygame.display.update()
scores = np.append(scores, score)
np.savetxt("scores.txt", scores)
pygame.quit()
sys.exit()
1 Answer 1
imports
Pep8 asks that you organize imports in the usual way. Don't bother learning the rules. Just use "$ isort *.py" and be done with it, move on to more pressing concerns.
comment on the why, not the how
# Defines window size
win_size = (900, 600)
You are saying what the code already eloquently states. Just elide this comment.
If you feel "win" is too cryptic,
maybe spell out window_size,
but the identifier is fine as-is.
Your identifiers like game_over_font are great, BTW.
A valuable comment would explain that this is the right size for use on a certain display in the living room, or so an Emacs buffer will also comfortably fit on the screen without overlap. Or that going any bigger makes it hard to maintain a 60 FPS frame rate.
Similarly here:
projectile_speed = 7 # Defines projectile speed
pathlib
scores = np.loadtxt(str(Path().absolute()) + "/scores.txt"). ...
That would be more naturally expressed as
....loadtxt(Path("scores.txt").resolve())
It is seldom that we want .absolute() -- prefer .resolve().
docstring
# AABB Collision implementation
def aabb(r1, r2):
This is a helpful comment.
It would have been better as a """docstring""",
on the line following def.
It would have been much better if it spelled out that we're doing axis-aligned bounding-box collision detection, perhaps citing the URL of a webpage that explains OBB vs AABB.
return r1.x < r2.x + r2.width and r1.x + r1.width > r2.x and r1.y < r2.y + r2.height and r1.y + r1.height > r2.y
This is illegible as written. Maybe you can discern its meaning, but I can't. Use black to format your code:
return (
r1.x < r2.x + r2.width
and r1.x + r1.width > r2.x
and r1.y < r2.y + r2.height
and r1.y + r1.height > r2.y
)
Now it's transparently obvious what's going on here.
meaningful identifier
# Displays game over content if input(collision) is true
def game_over(val):
if val:
Thank you for the valuable comment.
But why did we even need one?
Couldn't the code have said that?
The name val conveys nothing to the reader.
When I read the signature, I was looking for a type annotation
that would help explain the nature of val.
But then we see if val: and
it seems def game_over(val: bool) -> None:
could be a plausible signature.
But let's think about the Public API we're defining here.
Should the whole body be indented one level? Maybe.
Or perhaps we shouldn't even call game_over()
until the game is over.
Consider ditching the if, unconditionally returning False,
and alter the caller in this way:
running = not(player.collision()) or game_over()
We rely on short-circuite evaluation of or.
As long as we're collision free, we never evaluate
the "game over" disjunct.
But as long as we're changing the design,
why have "over" in the middle of the display loop at all?
Just assign
running = not player.collision()
and put the game over call after the while running: loop.
tuple unpack
These expressions are less convenient than they could be:
window.blit(... , (win_size[0]/2 - game_over_text.get_width()/2, win_size[1]/2 - game_over_text.get_height()/2))
The [0] and [1] subscripts are needlessly cryptic.
Define a pair of width and height temp vars:
win_w, win_h = win_size
We don't have to, but while we're at it we might shrink the spelling of that other expression, and go for parallel construction:
txt_w, txt_h = game_over_text.get_width(), game_over_text.get_height()
color names
....render("GAME OVER!", True, (0, 0, 0))
This is not terrible; it seems fairly obvious we're rendering in black.
....render("New Highscore!", True, (45, 45, 45))
....render(str(score), True, (110, 110, 110))
Ok, now we've gone too far. Define the first magic number as BLACK, and invent two new names, maybe LIGHT_GREY and DARK_GREY.
Also define PAUSE = 1000, as we use it twice and should slow down by similar amounts if we ever increase the delay.
object vs Object
I guess this is kind of OK?
I mean, they are distinct identifiers.
But it seems too generic, too close to python's core object class.
class Object:
global objects
objects = []
I would prefer to see it phrased:
objects = []
class Object:
and we don't even need a global declaration for objects.append().
A better name for the class might be Drawable.
In most game applications the conventional name would be Sprite.
Oh, lookit dat, you even mention sprite in the Projectile ctor.
Also, do we really need objects at module scope?
Couldn't the class hold it?
sgn()
Python is slightly odd among languages in that it lacks a sgn(y) function, for historic reasons. (There are corner cases, and consensus did not emerge for a universally useful way to handle them for all use cases.)
self.dir = -1 * (x/abs(x)) # Calucates required sprite direction based on input value
You should use copysign:
self.dir = -1 * math.copysign(1, x)
This more clearly conveys Author's Intent, without danger of exploding with ZeroDivisionError.
extract helper
This is an unfortunate one-liner:
def update_all(self):
valid_projectiles = [projectile for projectile in projectiles if (projectile.x > 0 - projectile.width and projectile.dir == -1) or (projectile.x < win_size[0] and projectile.dir == 1)] # Projectiles within bounds
Not even black can save it:
valid_projectiles = [
projectile
for projectile in projectiles
if (projectile.x > 0 - projectile.width and projectile.dir == -1)
or (projectile.x < win_size[0] and projectile.dir == 1)
]
Package up a helper predicate:
def is_valid(projectile) -> bool:
return (projectile.x > 0 - projectile.width and projectile.dir == -1) or (
projectile.x < win_size[0] and projectile.dir == 1
)
Now the caller can simply say:
for projectile in filter(is_valid, projectiles):
Also, creating a list so we can have a .pop() side effect
is not the best way to reveal Author's Intent.
Prefer an explicit for loop to produce the side effect,
rather than a list comprehension.
choose specific names
def set_bounds(self):
This is a pretty good identifier.
But it sounds like it's filling in the size of a wall or something.
If it was named enforce_bounds it would be a little clearer
that it's about how sprites interact with walls.
Consider rephrasing it in terms of min() and max(), e.g.
self.x = max(0, self.x)
I had many comments. I always do. But on the whole this game is written in a clear and straightforward style.
Keep up the good work!
win_sizeandprojectile_speed. Everything else needs to be moved into functions and classes. Move all of your top-level code intomain(). Once that's all done (as an edit to this question before any answers are posted, or in a new question if answers are posted on this question) I'd be happy to review it. \$\endgroup\$