0
\$\begingroup\$

Please review my Python Alien Blitz Game code.

There are several points I'm keen to hear about.

  • I used globals. Was that an evil choice given the context? In the past when I've tried to refactor these kinds of programs to pass arguments instead, I've got in a real muddle keeping track of them all.

  • Is this code at a level of complexity where not using OOP is "the wrong choice"?

  • How well did I handle the timer callbacks?

  • There was an issue with the bomb still dropping after a level was completed or there was a crash, and it carried over to the next round - I stopped this with by using the playing flag. Was that a good approach?

  • How good is my coding in general? Is the style consistent? Have I made any obvious or subtle errors or bad choices?

The very most helpful thing for me would be to see how a more experienced programmer would refactor my code. It would be great to see a version which has no globals but is still procedural, and a well-implemented OOP version. I know that is a big ask.

There is a repo containing the sounds as well as the program here:

https://github.com/Robin-Andrews/Alien-Blitz-Retro-Game-with-Python-Turtle-Graphics

Many thanks in advance for any feedback.

# alien_blitz.py

import turtle
import random

try:
    import playsound  # Not part of standard Library.

    SOUND = True
except ImportError:
    SOUND = False

NUM_TOWERS = 20
MAX_TOWER_HEIGHT = 10
CURSOR_SIZE = 20
PLANE_DELAY = 40
BOMB_DELAY = 40
WIDTH = 800
HEIGHT = 600
cell_colors = ["black", "dark green", "brown"]


def move_plane():
    global playing
    new_pos = (plane.xcor(), plane.ycor())
    if new_pos[0] > width // 2:
        plane.goto(- width // 2, plane.ycor() - size)
    else:
        plane.goto(plane.xcor() + 12, plane.ycor())

    if check_plane_tower_collision():
        playing = False
        restart(new_level=False)
    elif check_player_wins_level():
        restart(new_level=True)
    else:
        screen.update()
        turtle.ontimer(move_plane, PLANE_DELAY)


def check_player_wins_level():
    if score >= winning_score:
        player_wins_level()
        return True
    return False


def player_wins_level():
    update_score_display()
    if SOUND:
        playsound.playsound("victory.wav")


def check_plane_tower_collision():
    for tower in towers:
        for cell in tower:
            if plane.distance(cell) <= size / 2 + 10:  # Half cell size + half plane height
                plane_tower_collision()
                return True
    return False


def plane_tower_collision():
    bomb.hideturtle()  # If present when plane crashes
    plane.color("red")
    screen.update()
    if SOUND:
        playsound.playsound("plane_crash.wav")


def check_bomb_tower_collision():
    if playing:
        for tower in towers:
            for cell in tower:
                if bomb.distance(cell) <= size / 2 + 5:  # Half cell size + half bomb size
                    bomb_tower_collision(cell)
                    return True
        return False


def bomb_tower_collision(cell):
    global score, high_score
    if SOUND:
        playsound.playsound("bombed.wav", False)
    cell.setx(-1000)
    cell.clear()
    score += 10
    if score > high_score:
        high_score = score
    update_score_display()


def start_bomb_drop():
    # Prevent further key presses until drop is finished tp prevent event stacking.
    screen.onkey(None, "space")
    bomb.goto(plane.xcor(), plane.ycor())
    bomb.showturtle()
    __continue_bomb_drop()


def __continue_bomb_drop():
    global playing
    bomb.goto(bomb.xcor(), bomb.ycor() - 12)
    if check_bomb_tower_collision() or bomb.ycor() < - height // 2 or not playing:
        stop_bomb_drop()
    else:
        turtle.ontimer(__continue_bomb_drop, BOMB_DELAY)


def stop_bomb_drop():
    bomb.hideturtle()
    # It's now safe to allow another bomb drop, so rebind keyboard event.
    screen.onkey(start_bomb_drop, "space")


def update_score_display():
    pen.clear()
    pen.write("Score:{:2} High Score:{:2}".format(score, high_score), align="center", font=("Courier", 24, "normal"))


def get_towers():
    result = []
    for col in range(-NUM_TOWERS // 2, NUM_TOWERS // 2):
        tower = []
        for level in range(random.randrange(1, MAX_TOWER_HEIGHT + 1)):
            block = turtle.Turtle(shape="square")
            block.shapesize(size / CURSOR_SIZE)
            block.color(random.choice(cell_colors))
            block.penup()
            block.goto(col * size + offset, - height // 2 + level * size + offset)
            tower.append(block)
        result.append(tower)
    return result


def setup():
    global screen, plane, bomb, pen, high_score, size, offset, height, width, score
    # Screen
    screen = turtle.Screen()
    screen.title("Alien Blitz")
    screen.setup(WIDTH, HEIGHT)
    screen.bgcolor("dark blue")
    screen.listen()
    screen.onkey(start_bomb_drop, "space")
    screen.tracer(0)

    # MISC.
    width = screen.window_width() - 50
    height = screen.window_height() - 50
    size = width / NUM_TOWERS  # Size of tower cells in pixels
    offset = (NUM_TOWERS % 2) * size / 2 + size / 2  # Center even and odd cells

    # Plane
    plane = turtle.Turtle(shape="triangle", visible=False)
    plane.color("yellow")
    plane.shapesize(20 / CURSOR_SIZE, 40 / CURSOR_SIZE)
    plane.penup()
    plane.goto(- width // 2, height // 2)
    plane.showturtle()

    # Bomb
    bomb = turtle.Turtle(shape="circle")
    bomb.hideturtle()
    bomb.color("red")
    bomb.shapesize(0.5)
    bomb.penup()

    # Score Display
    pen = turtle.Turtle()
    pen.hideturtle()
    pen.color("white")
    pen.penup()
    pen.goto(0, 260)

    # Initialise high score
    high_score = 0


def restart(new_level=False):
    global score, high_score, winning_score, towers, playing
    #  Towers list does not exist on first call.
    try:
        for tower in towers:
            for cell in tower:
                cell.setx(-1000)
                cell.clear()
    except NameError:
        pass
    plane.color("yellow")
    towers = get_towers()
    # Here we handle the score for different scenarios for restarting the game - crashed plane or completed level.
    if not new_level:
        score = 0
        winning_score = sum(len(row) for row in towers) * 10
    else:
        winning_score += sum(len(row) for row in towers) * 10
    update_score_display()
    plane.goto(- width // 2, height // 2)
    bomb.goto(- width // 2, height // 2)
    playing = True
    screen.update()
    move_plane()


def main():
    setup()
    restart()


if __name__ == "__main__":
    main()
    turtle.done()

```
\$\endgroup\$
1
  • \$\begingroup\$ With the little authority I can be given, I wouldn't insert spaces in - variable to negate variables, nor would I do it around / and // when dividing variables by constants, i.e. I'd write -width or size/2 or -width//3. While it is important to let your mathematical expressions breath, there is no point in spacing them out too much because it then becomes harder to find the important parts. \$\endgroup\$
    – RGS
    Commented Apr 2, 2020 at 18:27

1 Answer 1

1
\$\begingroup\$

I'd say that yes, using globals was an evil choice, and I think that splitting this functionality into objects would be a good move.

As the original author you might have found it easier to just keep track of the globals in your head and not have to type them as arguments to each function, but as a reader, it makes it very hard to follow the changes in state between functions.

Not having to pass large numbers of variables as arguments between functions is where OOP can help you. (Note: I always think of OOP as being a means to an end and do not advocate shoving it in without a purpose -- come at me, Java bros! In this case, the "end" is being able to better organize all your bits and bobs of state.) On first blush, my approach to this would be to have two objects: a "game state" object (that tracks where everything is in the game and implements the "rules") and a "game UI" object (that manages all of the interface business, i.e. the screen drawing and the keypresses). So instead of one big pile of globals, you have two objects that hold all that data; your "setup" function creates and returns those two objects and then the rest of your game logic operates on/within them. You'd probably be able to turn some of your functions into class methods (as a rule of thumb, anything that modifies one of those pieces of state should probably live inside that class).

Once you'd finished that refactoring, my next recommendation would be to look at functions that don't operate purely on one object or the other, and then split those up -- separate your game logic from your UI. Basically you want each function to do one thing in one "layer" of your app, and keep responsibilities separated; this is the concept of "model/view" in a nutshell.

\$\endgroup\$

Your Answer

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

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