patternpythonModerate
Simple top down shooter game
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
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
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
For example, instead of:
(which are ALL in the
This would mean, instead of using
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...
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:
This has several advantages;
-
Each thing is specified once. Imagine if you moved our images to
-
You change the name/signature of the
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
```
def loadSprite(path, x, y):
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.