5
\$\begingroup\$

I'm trying to make a simple game for a school project. Do you have any tips on how to simplify and beautify the code?

GitHub

import pygame as py
import os

py.init ()

screen = py.display.set_mode ((800,600))



bg = py.image.load('game-assets-game-background-sidescroller.png')
clock = py.time.Clock ()


idle = [py.image.load(os.path.join('player','Idle (1).png')),py.image.load(os.path.join('player','Idle (2).png')),py.image.load(os.path.join('player','Idle (3).png')),py.image.load(os.path.join('player','Idle (4).png')),py.image.load(os.path.join('player','Idle (5).png')),py.image.load(os.path.join('player','Idle (6).png')),py.image.load(os.path.join('player','Idle (7).png')),py.image.load(os.path.join('player','Idle (8).png')),py.image.load(os.path.join('player','Idle (9).png')),py.image.load(os.path.join('player','Idle (10).png'))]
run_right = [py.image.load(os.path.join('player','Run (1).png')),py.image.load(os.path.join('player','Run (2).png')),py.image.load(os.path.join('player','Run (3).png')),py.image.load(os.path.join('player','Run (4).png')),py.image.load(os.path.join('player','Run (5).png')),py.image.load(os.path.join('player','Run (6).png')),py.image.load(os.path.join('player','Run (7).png')),py.image.load(os.path.join('player','Run (8).png'))]
jump = [py.image.load(os.path.join('player','Jump (1).png')),py.image.load(os.path.join('player','Jump (2).png')),py.image.load(os.path.join('player','Jump (3).png')),py.image.load(os.path.join('player','Jump (4).png')),py.image.load(os.path.join('player','Jump (5).png')),py.image.load(os.path.join('player','Jump (6).png')),py.image.load(os.path.join('player','Jump (7).png')),py.image.load(os.path.join('player','Jump (8).png')),py.image.load(os.path.join('player','Jump (9).png')),py.image.load(os.path.join('player','Jump (10).png'))]
shoot_idle = [py.image.load(os.path.join('player','Shoot (1).png')),py.image.load(os.path.join('player','Shoot (2).png')),py.image.load(os.path.join('player','Shoot (3).png')),py.image.load(os.path.join('player','Shoot (4).png'))]
shoot_run = [py.image.load(os.path.join('player','RunShoot (1).png')),py.image.load(os.path.join('player','RunShoot (2).png')),py.image.load(os.path.join('player','RunShoot (3).png')),py.image.load(os.path.join('player','RunShoot (4).png')),py.image.load(os.path.join('player','RunShoot (5).png')),py.image.load(os.path.join('player','RunShoot (6).png')),py.image.load(os.path.join('player','RunShoot (7).png')),py.image.load(os.path.join('player','Run (8).png'))]

class player(object):
    def __init__(self,x,y,width,lenght):
        self.x = x
        self.y = y
        self.width = width
        self.lenght = lenght
        self.vel = 5
        self.right = False
        self.left = False
        self.standing = True
        self.idlecount = 0
        self.runcount = 0
        self.jumping = False
        self.jumpcount = 14
        self.direction = 1
        self.jumpingcount = 0
        self.shooting = False
        self.shootingcount = 0

    def draw (self,screen):
        if self.idlecount + 1 >= 30:
            self.idlecount = 0

        if self.runcount + 1 >= 24:
            self.runcount = 0

        if self.jumpingcount + 1 >= 31:
            self.jumpingcount = 0

        if self.shootingcount + 1 >= 9:
            self.shootingcount = 0

        if not (self.jumping):
            if not (self.standing):

                if self.right:
                    screen.blit (run_right[self.runcount//3],(self.x,self.y))
                    self.runcount += 1

                elif self.left:
                    screen.blit (run_left[self.runcount//3],(self.x,self.y))
                    self.runcount += 1

            else:
                if self.shooting:
                    if self.direction == 1:
                        screen.blit (shoot_idle[self.shootingcount//2],(self.x,self.y))
                        self.shootingcount += 1

                    elif self.direction == -1:
                        screen.blit (shoot_idle2[self.shootingcount//2],(self.x,self.y))
                        self.shootingcount += 1

                elif self.direction == 1:
                    screen.blit (idle[self.idlecount//3],(self.x,self.y))
                    self.idlecount += 1

                elif self.direction == -1:
                    screen.blit (idle2[self.idlecount//3],(self.x,self.y))
                    self.idlecount += 1

        else:
            if self.direction == 1:
                screen.blit (jump[self.jumpingcount//3],(self.x,self.y))
                self.jumpingcount += 1
                self.runcount = 0

            elif self.direction == -1:
                screen.blit (jump2[self.jumpingcount//3],(self.x,self.y))
                self.jumpingcount += 1
                self.runcount = 0



pows = [py.image.load(os.path.join('player','Bullet_000.png')),py.image.load(os.path.join('player','Bullet_001.png')),py.image.load(os.path.join('player','Bullet_002.png')),py.image.load(os.path.join('player','Bullet_003.png')),py.image.load(os.path.join('player','Bullet_004.png'))]

class bulletss (object):
    def __init__(self,x,y,facing):
        self.x = x
        self.y = y
        self.facing = facing
        self.vel = 10 * facing
        self.shootcount = 0
        self.lenght = 50
        self.width = 50

    def draw(self,win):

        if self.shootcount + 1 == 12:
            self.shootcount = 0

        if self.facing == 1:
            screen.blit (pows[self.shootcount//3],(self.x+25,self.y-25))
            self.shootcount += 1

        elif self.facing == -1:
            screen.blit (pows2[self.shootcount//3],(self.x-75,self.y-25))
            self.shootcount += 1


def drawGameScreen ():
    screen.blit(bg,(0,0))
    man.draw (screen)
    for bullet in bullets:
        bullet.draw (screen)


    py.display.update ()

##main loop

man = player (100,430,100,100)
game = True
bullets = []
shootloop = 0

idle = [(py.transform.scale(idle[0],(man.width,man.lenght))),(py.transform.scale(idle[1],(man.width,man.lenght))),(py.transform.scale(idle[2],(man.width,man.lenght))),(py.transform.scale(idle[3],(man.width,man.lenght))),(py.transform.scale(idle[4],(man.width,man.lenght))),(py.transform.scale(idle[5],(man.width,man.lenght))),(py.transform.scale(idle[6],(man.width,man.lenght))),(py.transform.scale(idle[7],(man.width,man.lenght))),(py.transform.scale(idle[8],(man.width,man.lenght))),(py.transform.scale(idle[9],(man.width,man.lenght)))]
run_right = [(py.transform.scale(run_right[0],(man.width,man.lenght))),(py.transform.scale(run_right[1],(man.width,man.lenght))),(py.transform.scale(run_right[2],(man.width,man.lenght))),(py.transform.scale(run_right[3],(man.width,man.lenght))),(py.transform.scale(run_right[4],(man.width,man.lenght))),(py.transform.scale(run_right[5],(man.width,man.lenght))),(py.transform.scale(run_right[6],(man.width,man.lenght))),(py.transform.scale(run_right[7],(man.width,man.lenght)))]
jump = [(py.transform.scale(jump[0],(man.width,man.lenght))),(py.transform.scale(jump[1],(man.width,man.lenght))),(py.transform.scale(jump[2],(man.width,man.lenght))),(py.transform.scale(jump[3],(man.width,man.lenght))),(py.transform.scale(jump[4],(man.width,man.lenght))),(py.transform.scale(jump[5],(man.width,man.lenght))),(py.transform.scale(jump[6],(man.width,man.lenght))),(py.transform.scale(jump[7],(man.width,man.lenght))),(py.transform.scale(jump[8],(man.width,man.lenght))),(py.transform.scale(jump[9],(man.width,man.lenght)))]
run_left = [(py.transform.flip(run_right[0],True,False)),(py.transform.flip(run_right[1],True,False)),(py.transform.flip(run_right[2],True,False)),(py.transform.flip(run_right[3],True,False)),(py.transform.flip(run_right[4],True,False)),(py.transform.flip(run_right[5],True,False)),(py.transform.flip(run_right[6],True,False)),(py.transform.flip(run_right[7],True,False))]
idle2 = [(py.transform.flip(idle[0],True,False)),(py.transform.flip(idle[1],True,False)),(py.transform.flip(idle[2],True,False)),(py.transform.flip(idle[3],True,False)),(py.transform.flip(idle[4],True,False)),(py.transform.flip(idle[5],True,False)),(py.transform.flip(idle[6],True,False)),(py.transform.flip(idle[7],True,False)),(py.transform.flip(idle[8],True,False)),(py.transform.flip(idle[9],True,False))]
jump2 = [(py.transform.flip(jump[0],True,False)),(py.transform.flip(jump[1],True,False)),(py.transform.flip(jump[2],True,False)),(py.transform.flip(jump[3],True,False)),(py.transform.flip(jump[4],True,False)),(py.transform.flip(jump[5],True,False)),(py.transform.flip(jump[6],True,False)),(py.transform.flip(jump[7],True,False)),(py.transform.flip(jump[8],True,False)),(py.transform.flip(jump[9],True,False))]
shoot_idle = [(py.transform.scale(shoot_idle[0],(man.width,man.lenght))),(py.transform.scale(shoot_idle[1],(man.width,man.lenght))),(py.transform.scale(shoot_idle[2],(man.width,man.lenght))),(py.transform.scale(shoot_idle[3],(man.width,man.lenght)))]
shoot_idle2 = [(py.transform.flip(shoot_idle[0],True,False)),(py.transform.flip(shoot_idle[1],True,False)),(py.transform.flip(shoot_idle[2],True,False)),(py.transform.flip(shoot_idle[3],True,False))]
pows = [(py.transform.scale(pows[0],(50,50))),(py.transform.scale(pows[1],(50,50))),(py.transform.scale(pows[2],(50,50))),(py.transform.scale(pows[3],(50,50))),(py.transform.scale(pows[4],(50,50)))]
pows2 = [(py.transform.flip(pows[0],True,False)),(py.transform.flip(pows[1],True,False)),(py.transform.flip(pows[2],True,False)),(py.transform.flip(pows[3],True,False)),(py.transform.flip(pows[4],True,False))]


while game:
    clock.tick (30)

    if shootloop > 0:
        shootloop += 1

    if shootloop > 5:
        shootloop = 0

    for event in py.event.get():
        if event == py.QUIT:
            game = False

    keys = py.key.get_pressed ()

    for bullet in bullets:
        if bullet.x < 800 and bullet.x > 0:
            bullet.x += bullet.vel

        else:
            bullets.remove (bullet)




    if keys[py.K_RIGHT] and man.x <= 700:
        man.x += man.vel
        man.right = True
        man.left = False
        man.standing = False
        man.idlecount = 0
        man.direction = 1

    elif keys[py.K_LEFT] and man.x >= 0:
        man.x -= man.vel
        man.right = False
        man.left = True
        man.standing = False
        man.idlecount = 0
        man.direction = -1

    else:
        man.standing = True
        man.shooting = False

    if keys [py.K_SPACE] and shootloop == 0:
        if man.left:
            facing = -1

        elif man.right:
            facing = 1

        if len(bullets) < 5:
            man.standing = True
            man.shooting = True
            bullets.append(bulletss(round(man.x + man.lenght//2), round(man.y + man.lenght//2), facing))

        shootloop = 1

    if not(man.jumping):
        if keys[py.K_UP]:
            man.jumping = True
            man.right = False
            man.left = False
            man.standing = False
            man.walkcount = 0
    else:
        if man.jumpcount >= -14:
            neg = 1
            if man.jumpcount < 0:
                neg = -1
            man.y -= (man.jumpcount ** 2) * 0.2 * neg
            man.jumpcount -= 1
        else:
            man.jumping = False
            man.jumpcount = 14


    drawGameScreen ()
\$\endgroup\$
2
  • \$\begingroup\$ Just to note, I'm working on a very detailed answer. \$\endgroup\$
    – Graham
    Nov 10, 2018 at 22:42
  • \$\begingroup\$ Please mention what the game logic is! \$\endgroup\$
    – hjpotter92
    Nov 12, 2018 at 13:15

2 Answers 2

6
\$\begingroup\$

This is a big review, and it will be hard to cover everything, so I'll go with some main points and then a few tips (having started my own game in pygame as well, if you want to take a look - it's not perfect though - https://github.com/arthur-hav/hexrl).

  1. In terms of data structure, in your player you can trade multiple standing shooting jumping booleans for a single self.state which could be either "jumping", "shooting" etc.
  2. Once done, and have animations stored in a dictionary of list of images with keys being self.state values. This way, in your draw function you could do something in the lines of :

    animation = self.animations[self.state] count = self.counts[self.state] screen.blit(animation[count], (self.x, self.y))

Using data structures is a good way to avoid repeating yourself and keep the code short.

  1. Bullet and player can derivate from a same base class, they are game objects with sprites. As such, they always have an image, possibly animations, and a rect (pygame rect includes x, y, width and length). They can have also in common a generic display function, which blits to the screen. You can later add also the loading code to it, and maybe animation and physics code, but this is optional.
  2. Your main loop does way too much. Instead, you can have an update() method in your game objects. Each would behave accordingly, such as the bullet velocity. Same for input handling : since only the player reacts to input, it should have a move() function handling man.x += man.vel and so on.

Below are more bonus points:

  1. You can have game constants such as image names, animation max numbers etc. isolated from your classes and passed to init function. This is useful for clarity, separating data from code, and creating new instances with different behavior (e.g. monsters).
  2. Use an observer pattern for input handling. This is especially useful if you add a menu later on.
  3. Split the code in several files. For example one file with your game objects and one with the main loop and the input handling.
  4. General code : try to follow PEP8, which normalizes the number of lines to jump between each block and so on. No double jump in the middle of a function. There are automated tools which should guide you on that. It's also length, not lenght ;)
\$\endgroup\$
3
\$\begingroup\$

Note that this answer is unfinished, but I decided to post it so that the OP could benefit from the process, because Arthur Havlicek posted an answer. I intend to come back later and finish it (we'll see).

Spelling

You've spelled "length" as "lenght" a total of 38 times throughout your code (though most of those are unnecessarily redundant, which we'll get to in a moment). While you have been consistent in your misspelling, this could cause problems later if you wanted to write more code, or if someone else was modifying you code. To fix this, you can find-and-replace lenght to length (if your code editor of choice has that functionality, which it should.)


Sprite handling

Your handling of sprites provides a good case study for how you can generally improve your coding style to be more cohesive. I have split the refactoring into several steps.

1. Create DRY code

In the beginning you declare several lists of items that have unnecessary repetition. I recommend using the following list comprehensions with formatted string literals instead:

idle = [py.image.load(os.path.join('player', f'Idle ({i}).png')) for i in range(1,11)]
run_right = [py.image.load(os.path.join('player', f'Run ({i}).png')) for i in range(1,9)]
jump = [py.image.load(os.path.join('player', f'Jump ({i}).png')) for i in range(1,11)]
shoot_idle = [py.image.load(os.path.join('player', f'Shoot ({i}).png')) for i in range(1,5)]
shoot_run = [*(py.image.load(os.path.join('player', f'RunShoot ({i}).png')) for i in range(1,8)),
               py.image.load(os.path.join('player', 'Run (8).png'))]

Here's the rundown on formatted string literals: they are string preceded by an f, and anything within curly brackets is interpreted literally in Python, instead of as a string. So for example, f'Idle ({i}).png' replaces {i} with whatever the value of i is at the time of assignment. Here, the list comprehension loops through 1, 2, ..., 9, 10, so at various points in the list, the string becomes 'Idle (1)', 'Idle (2)' , ..., 'Idle (9)', 'Idle (10)'.

2. Move similar code together... for now

Code that has similar functionality should be kept together to make it more difficult to make inconsistent changes. I notice you declared the sprite list pows separately from the rest of the sprites. You can move that together with the rest of the sprites and apply the same type of loop:

# ...
pows = [py.image.load(os.path.join('player', f'Bullet_{i:0>3}.png')) for i in range(5)]

At this point it also pays to know more of the format specification mini language. here, the replacement field {i:0>3} means:

i        # Variable i
 :       # Begin specifying string formatting options
  0      # Pad with '0'
   >     # Right align the variable
    3    # Pad the variable to fill at least a 3-character width

But this is only the first step: there's still repetition between the lists. We could make a custom sprite import function to make this process more convenient. That leads us to:

3. Be consistent, unless you have a good reason

When your initial sprite loading code is all put together, I notice you use two different file naming formats:

  • SpriteType (1_INDEXED_NUM)
  • SpriteType_THREE_PADDED_0_INDEXED_NUM

(Three-padded means it fills up three digits, and N-Indexed means the numbers start from N.)

I would switch to a the following style:

  • SpriteType0_INDEXED_NUM

It's a good idea to 0-index, because it allows better interoperability with Python's sequence indexing: range(), which I've been using above, starts from 0 and excludes its stop argument; this allows one to easily specify ranges of a certain size: range(5) has 5 elements (0 through 4), and range(10) has 10 elements (0 through 9).

I've removed the space between the sprite name and number because it seems unnecessary. If sprite names in the future needed numbers, they could be spelled out to avoid confusion.

This will require changing all of your file names in that directory, but you can do that by going into the player directory and running a simple* script I hacked together and tested:

import os
import re

name_fmt = re.compile(r'''(?P<name>\w+)
                          (?P<underscore>_)?
                          (?(underscore)|[ ]\()
                          (?P<num>\d+)
                          (?(underscore)|\))
                          .png''', re.X)
unrecognized = []
for filename in os.listdir():
    match = name_fmt.fullmatch(filename)
    if match:
        name = match['name']
        num = int(match['num'])
        if not match['underscore']:
            num -= 1
        os.rename(filename, f'{name}{num}.png')
    else:
        unrecognized.append(filename)
print('The following files were not recognized as needing a format change:', ', '.join(f'"{f}"' for f in unrecognized))

* Unless you know regular expressions (and even if you do), the name_fmt bit is probably a bit complicated. But it was relatively easy for me to do, because I know regex very well. There are probably better ways to handle the file extension, but I created this script with speed of coding from memory in mind, especially since this answer will be very long, and I have will spend a chunk of time on other parts.

4. Standardizing sprite loading through functions (more DRY coding)

Then I would make the following functions:

def load_sprite(name, i):
    return py.image.load(os.path.join('player', f'{name} {i}.png'))

def load_sprites(name, num):
    return [load_sprite(name, i) for i in range(num)]

And apply them:

idle = load_sprites('Idle', 10)
run_right = load_sprites('Run', 8)
jump = load_sprites('Jump', 10)
shoot_idle = load_sprites('Shoot', 4)
shoot_run = load_sprites('RunShoot', 7) + [load_sprite('Run', 7)]
pows = load_sprites('Bullet', 5)

5. Utilize object oriented programming

Unfinished section.


Some PEP-8 Notes

PEP-8 is the style guide for Python, and we like following it because it makes code easier to read and understand. It's basically a way to ensure that Python coders have a set of style standards, e.g. naming, formatting, etc., so we can understand each other's code as easily as possible. I would recommend reading it thoroughly and trying your best to follow it. There are several things you can do to improve your compliance:

1. Names

PEP-8 says:

Class names should normally use the CapWords convention.

The naming convention for functions may be used instead in cases where the interface is documented and used primarily as a callable.

Note that there is a separate convention for builtin names: most builtin names are single words (or two words run together), with the CapWords convention used only for exception names and builtin constants.

This means you should rename your classes to Player and Bullet (because Bulletss is not an easy name to remember.)

Additionally:

Function names should be lowercase, with words separated by underscores as necessary to improve readability.

[...]

mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

So drawGameScreen should become draw_game_screen.

If you're using an IDE, it might allow you to rename your variables. Otherwise, you can highlight all the changes in your editor find function (to ensure you don't change anything you didn't want to) before replacing.

2. Avoid extraneous whitespace

PEP-8 says:

Avoid extraneous whitespace in the following situations:

  • Immediately before the open parenthesis that starts the argument list of a function call:

    • Yes: spam(1)

    • No: spam (1)

That means every time you call screen.blit, there should not be a space before the opening parenthesis of the method body. The same applies throughout the rest of your code; it starts at py.init (). As Arthur Havlicek mentioned, there should not be multiple lines after a block. Coming back around:


Practice DRY coding (II)

Generally, if you have statements of the form:

if conditional:
    # do something

    something_count += 1
else:
    # do something else
    
    something_count += 1

You can remove the something_count increment from the conditionals and just have it as a post-conditional increment:

if conditional:
    # do something
else:
    # do something else
something_count += 1

Don't use magic constants

Here's a conditional I see many times throughout your code: self.direction == 1. What is 1? Is 1 left or right? This is what programmers call a magic constant. You should try to avoid literal integer comparisons in your code unless the purpose is completely clear (and some would argue you shouldn't have them even then.) Instead, you should make variables called left and right for comparisons. But there's an even better way: use enums: derive a class from enum called Direction and then make it have the values LEFT and RIGHT.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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