Simple top down shooter game

Question

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/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)


def loadMap(path):
    wall_coords = []
    enemy_coords = []
    file = open(path, 'r')
    file = str(file.read())
    file = file.split('\n')
    for row in range(len(file)):
        for column in range(len(file[row])):
            if file[row][column] == '@':
                lead_coords = [column*block_s, row*block_s]
            if file[row][column] == '#':
                wall_coords.append([column*block_s, row*block_s])
            if file[row][column] == 'e':
                enemy_coords.append([column*block_s, row*block_s])
    return [lead_coords, wall_coords, enemy_coords]


def mainLoop():
    lead_dir = leadup
    enemy_dir = enemyup
    lead_x_change = 0
    lead_y_change = 0
    lead_coords, wall_coords, enemy_coords = loadMap('map.txt')
    lead_x, lead_y = lead_coords

    bullet_coords = []
    bullet_dir = bulletup
    bulletMotion = False

    gameOver = False
    while True:         
        while gameOver == True:
            screen.blit(gameover, (0, 0))
            pygame.display.update()
            for event in pygame.event.get():
                if event.type == pygame.KEYDOWN:
                    if event.key == pygame.K_n:
                        pygame.quit()
                        quit()
                    elif event.key == pygame.K_y:
                        mainLoop()

        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                pygame.quit()
                quit()
            if event.type == pygame.KEYDOWN:
                if event.key == pygame.K_LEFT:
                    lead_dir = leadleft
                    lead_x_change = -block_s
                    lead_y_change = 0
                elif event.key == pygame.K_RIGHT:
                    lead_dir = leadright
                    lead_x_change = block_s
                    lead_y_change = 0
                elif event.key == pygame.K_UP:
                    lead_dir = leadup
                    lead_y_change = -block_s
                    lead_x_change = 0
                elif event.key == pygame.K_DOWN:
                    lead_dir = leaddown
                    lead_y_change = block_s
                    lead_x_change = 0

                if event.key == pygame.K_z and bulletMotion == False:
                    if lead_dir == leadup:
                        bullet_coords.append(lead_x)
                        bullet_coords.append(lead_y)
                        bullet_dir = bulletup

                    elif lead_dir == leaddown:
                        bullet_coords.append(lead_x)
                        bullet_coords.append(lead_y)
                        bullet_dir = bulletdown

                    if lead_dir == leadright:
                        bullet_coords.append(lead_x)
                        bullet_coords.append(lead_y)
                        bullet_dir = bulletright

                    if lead_dir == leadleft:
                        bullet_coords.append(lead_x)
                        bullet_coords.append(lead_y)
                        bullet_dir = bulletleft
                    bulletMotion = True


            if event.type == pygame.KEYUP:
                if event.key == pygame.K_LEFT:
                    lead_x_change = 0
                    lead_y_change = 0
                elif event.key == pygame.K_RIGHT:
                    lead_x_change = 0
                    lead_y_change = 0
                elif event.key == pygame.K_UP:
                    lead_x_change = 0
                    lead_y_change = 0
                elif event.key == pygame.K_DOWN:
                    lead_x_change = 0
                    lead_y_change = 0

        if bulletMotion == True:                    
            if bullet_dir == bulletup:
                bullet_coords[1] -= block_s
            elif bullet_dir == bulletdown:
                bullet_coords[1] += block_s
            elif bullet_dir == bulletright:
                bullet_coords[0] += block_s
            elif bullet_dir == bulletleft:
                bullet_coords[0] -= block_s

            if bullet_coords in wall_coords:
                bullet_coords = []
                bulletMotion = False
            elif bullet_coords in enemy_coords:
                enemy_coords.remove(bullet_coords)

        if [lead_x + lead_x_change, lead_y + lead_y_change] in wall_coords:
            lead_x_change = 0
            lead_y_change = 0


        for coordinate in enemy_coords:
            direction = random.choice(['X', 'Y'])
            if direction == 'X':
                if coordinate[0] > lead_x:
                    if [coordinate[0] - block_s, coordinate[1]] in wall_coords:
                        continue
                    else:
                        enemy_dir = enemyleft
                        coordinate[0] -= block_s

                elif coordinate[0] < lead_x:
                    if [coordinate[0] + block_s, coordinate[1]] in wall_coords:
                        continue
                    else:
                        enemy_dir = enemyright
                        coordinate[0] += block_s

            elif direction == 'Y':
                if coordinate[1] > lead_y:
                    if [coordinate[0], coordinate[1] - block_s] in wall_coords:
                        continue
                    else:
                        enemy_dir = enemyup
                        coordinate[1] -= block_s

                elif coordinate[1] < lead_y:
                    if [coordinate[0], coordinate[1] + block_s] in wall_coords:
                        continue
                    else:
                        enemy_dir = enemydown
                        coordinate[1] += block_s

        if [lead_x, lead_y] in enemy_coords:
            gameOver = True

        lead_x += lead_x_change
        lead_y += lead_y_change

        screen.blit(background, (0, 0))
        screen.blit(lead_dir, (lead_x, lead_y))
        for i in wall_coords:
            screen.blit(wall, (i[0], i[1]))
        for i in enemy_coords:
            screen.blit(enemy_dir, (i[0], i[1]))
        if bulletMotion == True:
            screen.blit(bullet_dir, (bullet_coords[0], bullet_coords[1]))

        pygame.display.update()
        clock.tick(FPS)
mainLoop()

Very nice first question! The code is obviously written in a procedural style (as opposed to OOP, for example). I am interested if this was intended/desired? Obviously there are MANY recommendations that could be made about making this code more OOP-like, but procedural code is perfectly valid in Python. — Tersosauros, Commented Mar 4, 2016 at 2:19
@Tersosauros The procedural style was not particularly desired, as I'm not familiar with OOP. If OOP is better organized/more efficient in this scenario, etc, then examples on how to do so would be appreciated. — user99331, Commented Mar 4, 2016 at 2:25

Community · Accepted Answer · 2017-03-16 16:38:25Z

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):
    return pygame.transform.scale((pygame.image.load(path).convert_alpha()),(x,y))

to something which either uses default arguments (x and y defaulting to block_s), or which allows either a tile size (square) OR x and Y (rectangle) to be supplied. For example:

 def loadSprite(path, x, y=None):
    return pygame.transform.scale((pygame.image.load(path).convert_alpha()),(x,x if y is None else y))

On to the OOP!

Your program currently has a number (7, actually) of global variables:

FPS
clock
block_s
screen_w
screen_h
screen
font

(That is, excluding the 8 colors and 18 sprites that I already mentioned).

These pretty much ALL belong in some sort of class. Probably named something like Game, World, Engine, etc. This would provide a place for all of the data (variables) and behavior (functions, called methods once they're in a class) of the game itself.

Using the hierarchies idea from the decomposition concept, this Game class could hierarchically contain (i.e. be the parent of,) the sprites (or images) and colors data structures created earlier.

The line pygame.init() would go into the constructor, rather than being in the main program body.

Each of the 3 global functions; message, loadMap, and mainLoop could then be made into methods of that class.

_{Note: the playMusic and loadSprite global functions may or may not be promoted to methods. They seem to me more like helper functions for PyGame, rather than explicit behaviors of your Game/World.}

Your `main()`... monster?

This function is 153 lines of code, with a cyclomatic complexity of around 54! I really don't even want to begin on it until the program structure (everything else I've talked about) is improved.

However, I will just say that you CLEARLY need decomposition here! Methods should always have a complexity less than 7, so this function would need to be decomposed into around 8 other (smaller) functions. Although, from scanning over it and looking at all your loop conditions (like while True, etc) I think you can probably made it less complex.

As I said at the start of this answer, this may be something where you need/desire/end up with multiple reviews. You can refactor your code and post another question with the new (OOP structured) version (then put a comment on this question pointing to it, etc).

\$\begingroup\$ What an explanation! It deserves a lot of upvotes \$\endgroup\$
– Iván Rodríguez Torres
Commented Mar 4, 2016 at 10:53 — Iván Rodríguez Torres, Commented Mar 4, 2016 at 10:53

Stack Exchange Network

Simple top down shooter game

1 Answer 1

Decomposition

Don't Repeat Yourself (DRY)

On to the OOP!

Your `main()`... monster?

Your Answer

Hot Network Questions

1 Answer 1

On to the OOP!

Your main()... monster?

Your Answer

Sign up or log in

Post as a guest

Related

Your `main()`... monster?