HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonModerate

Simple top down shooter game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simpletopgamedownshooter

Problem

This one of my first Python games in PyGame I decided to make for fun. Basically, you control a from top down perspective and shoot people in a maze.

This is the rewrite of the original code, and I'm aware that it is still probably very inefficient and has does many things frowned upon by many such as many repeated if statements, etc (that I don't know how to fix).

I would like to know your suggestions on improvements I can make, as well as point out my bad habits and tell me how to fix them.

```
import pygame
import random

pygame.init()

black = [0, 0, 0]
white = [255, 255, 255]
red = [255, 0, 0]
orange = [255, 102, 0]
yellow = [255, 255, 0]
green = [0, 255, 0]
blue = [0, 0, 255]
purple = [128, 0, 128]

FPS = 15
clock = pygame.time.Clock()

block_s = 30
screen_w, screen_h = 900, 600
assert screen_w % block_s == 0, 'Window width must be multiple of tile side.'
assert screen_h % block_s == 0, 'Window height must be multiple of tile side.'
screen = pygame.display.set_mode((screen_w, screen_h), 0, 32)
pygame.display.set_caption('Communism Simulator 2')

font = pygame.font.SysFont(None, 24)

def playMusic(path):
song = pygame.mixer.Sound(random.choice(path))
song.play()

def message(msg, color, x, y):
text = font.render(msg, True, color)
screen.blit(text, [x, y])

def loadSprite(path, x, y):
return pygame.transform.scale((pygame.image.load(path).convert_alpha()),(x,y))

gameover = loadSprite('images/gameover.png', screen_w, screen_h)
background = loadSprite('images/background.png', screen_w, screen_h)
menu = loadSprite('images/menu.png', screen_w, screen_h)

item = loadSprite('images/item.png', block_s, block_s)
vodka = loadSprite('images/powerup.png', block_s, block_s)
wall = loadSprite('images/wall.png', block_s, block_s)

leadup = loadSprite('images/leadup.png', block_s, block_s)
leaddown = loadSprite('images/leaddown.png', block_s, block_s)
leadright = loadSprite('images/leadright.png', block_s, block_s)
leadleft = loadSprite('images/leadle

Solution

Alright, this may be something where you need/desire/end up with multiple reviews - since changing this from it's current (procedural) style to OOP is (very) unlikely to be something someone new to OOP gets right first time.

But, there are certain aspects of this code that could be made better even in a procedural style - so I'll start with these...

Decomposition

Decomposition, or "functional decomposition" is important in all structured programming paradigms (which includes both OOP and procedural programming).
One of the most elementary steps in this process is to construct your programs using modularity and hierarchies.

Structurally, using different scopes for different things is important in creating conceptually seperated 'modules'.
For example, in your program the variable purple (a color, obviously), is in the same scope as the variable vodka (in image for a power-up item). Logically speaking, an image and a color do not belong in the same place - they should be separated (into 'modules', of some logical nature).

For example, instead of:

black = [0, 0, 0]
white = [255, 255, 255]
red = [255, 0, 0]
orange = [255, 102, 0]
yellow = [255, 255, 0]
green = [0, 255, 0]
blue = [0, 0, 255]
purple = [128, 0, 128]


(which are ALL in the __main__ scope,) you could have used a dictionary (Python's dict) data structure, like this:

colors={
    "black" : [0, 0, 0],
    "white" : [255, 255, 255],
    "red" : [255, 0, 0],
    "orange" : [255, 102, 0],
    "yellow" : [255, 255, 0],
    "green" : [0, 255, 0],
    "blue" : [0, 0, 255],
    "purple" : [128, 0, 128]
}


This would mean, instead of using yellow, you would now use colors["yellow"]. This not only helps you structure your program's code, but also makes it clearer what something is. This is probably not a big deal for "yellow", but when scrolling through and seeing vodka, rather than images["vodka"], it can create confusion.

Note:   in an object oriented design, you may wish to decompose these to a class or subclass rather than using a dictionary.

This same decomposition concept could also be used in your twenty lines of sprite loading!

Don't Repeat Yourself (DRY)

Some of your code, for example the following block of sprite definitions, is VERY repetitive...

gameover = loadSprite('images/gameover.png', screen_w, screen_h)
background = loadSprite('images/background.png', screen_w, screen_h)
menu = loadSprite('images/menu.png', screen_w, screen_h)

item = loadSprite('images/item.png', block_s, block_s)
vodka = loadSprite('images/powerup.png', block_s, block_s)
wall = loadSprite('images/wall.png', block_s, block_s)

leadup = loadSprite('images/leadup.png', block_s, block_s)
leaddown = loadSprite('images/leaddown.png', block_s, block_s)
leadright = loadSprite('images/leadright.png', block_s, block_s)
leadleft = loadSprite('images/leadleft.png', block_s, block_s)

enemyup = loadSprite('images/enemyup.png', block_s, block_s)
enemydown = loadSprite('images/enemydown.png', block_s, block_s)
enemyright = loadSprite('images/enemyright.png', block_s, block_s)
enemyleft = loadSprite('images/enemyleft.png', block_s, block_s)

bulletup = loadSprite('images/bulletup.png', block_s, block_s)
bulletdown = loadSprite('images/bulletdown.png', block_s, block_s)
bulletright = loadSprite('images/bulletright.png', block_s, block_s)
bulletleft = loadSprite('images/bulletleft.png', block_s, block_s)


Generally, any time your have many lines like this that are largely the same, you're better off using some sort of loop to construct a data structure to contain them. For example:

img_dir = "images/"
screens = {
    "gameover" : 'gameover.png',
    "background" : 'background.png',
    "menu" : 'menu.png'
}
for screen in screens.keys():
    screens[screen]=loadSprite(img_dir+screens[screen], screen_w, screen_h)

sprites = {
    "item" : 'item.png', "vodka" : 'powerup.png', "wall" : 'wall.png',
    "leadup" : 'leadup.png', "leaddown" : 'leaddown.png', "leadright" : 'leadright.png', "leadleft" : 'leadleft.png', 
    "enemyup" : 'enemyup.png', "enemydown" : 'enemydown.png', "enemyright" : 'enemyright.png', "enemyleft" : 'enemyleft.png',
    "bulletup" : 'bulletup.png', "bulletdown" : 'bulletdown.png', "bulletright" : 'bulletright.png', "bulletleft" : 'bulletleft.png'
}
for sprite in sprites.keys():
    sprites[sprite]=loadSprite(img_dir+sprites[sprite], block_s, block_s)


This has several advantages;

-
Each thing is specified once. Imagine if you moved our images to imgs/ rather than images/ - this way you need only change 1 variable (img_dir) on 1 line (rather than editing 20 lines).

-
You change the name/signature of the loadSprite function. 2 lines to edit rather than 20, etc.

It is fairly apparent that your game world is using square tiles, thus the x and y of the sprites (as oppose to the screens, such as 'background"
You may also wish to alter your loadSprite function from:

```
def loadSprite(path, x, y):

Code Snippets

black = [0, 0, 0]
white = [255, 255, 255]
red = [255, 0, 0]
orange = [255, 102, 0]
yellow = [255, 255, 0]
green = [0, 255, 0]
blue = [0, 0, 255]
purple = [128, 0, 128]
colors={
    "black" : [0, 0, 0],
    "white" : [255, 255, 255],
    "red" : [255, 0, 0],
    "orange" : [255, 102, 0],
    "yellow" : [255, 255, 0],
    "green" : [0, 255, 0],
    "blue" : [0, 0, 255],
    "purple" : [128, 0, 128]
}
gameover = loadSprite('images/gameover.png', screen_w, screen_h)
background = loadSprite('images/background.png', screen_w, screen_h)
menu = loadSprite('images/menu.png', screen_w, screen_h)

item = loadSprite('images/item.png', block_s, block_s)
vodka = loadSprite('images/powerup.png', block_s, block_s)
wall = loadSprite('images/wall.png', block_s, block_s)

leadup = loadSprite('images/leadup.png', block_s, block_s)
leaddown = loadSprite('images/leaddown.png', block_s, block_s)
leadright = loadSprite('images/leadright.png', block_s, block_s)
leadleft = loadSprite('images/leadleft.png', block_s, block_s)

enemyup = loadSprite('images/enemyup.png', block_s, block_s)
enemydown = loadSprite('images/enemydown.png', block_s, block_s)
enemyright = loadSprite('images/enemyright.png', block_s, block_s)
enemyleft = loadSprite('images/enemyleft.png', block_s, block_s)

bulletup = loadSprite('images/bulletup.png', block_s, block_s)
bulletdown = loadSprite('images/bulletdown.png', block_s, block_s)
bulletright = loadSprite('images/bulletright.png', block_s, block_s)
bulletleft = loadSprite('images/bulletleft.png', block_s, block_s)
img_dir = "images/"
screens = {
    "gameover" : 'gameover.png',
    "background" : 'background.png',
    "menu" : 'menu.png'
}
for screen in screens.keys():
    screens[screen]=loadSprite(img_dir+screens[screen], screen_w, screen_h)

sprites = {
    "item" : 'item.png', "vodka" : 'powerup.png', "wall" : 'wall.png',
    "leadup" : 'leadup.png', "leaddown" : 'leaddown.png', "leadright" : 'leadright.png', "leadleft" : 'leadleft.png', 
    "enemyup" : 'enemyup.png', "enemydown" : 'enemydown.png', "enemyright" : 'enemyright.png', "enemyleft" : 'enemyleft.png',
    "bulletup" : 'bulletup.png', "bulletdown" : 'bulletdown.png', "bulletright" : 'bulletright.png', "bulletleft" : 'bulletleft.png'
}
for sprite in sprites.keys():
    sprites[sprite]=loadSprite(img_dir+sprites[sprite], block_s, block_s)
def loadSprite(path, x, y):
    return pygame.transform.scale((pygame.image.load(path).convert_alpha()),(x,y))

Context

StackExchange Code Review Q#121850, answer score: 10

Revisions (0)

No revisions yet.