Skip to main content
deleted 2 characters in body
Source Link
J_H
  • 42.3k
  • 3
  • 38
  • 157

It's not like there wasthere's something else that was going to care about $PWD. But I'm afraid that global variables make me a little nervous, like time.time(), PRNG state, and PWD.

It's not like there was something else that was going to care about $PWD. But I'm afraid that global variables make me a little nervous, like time.time(), PRNG state, and PWD.

It's not like there's something else that was going to care about $PWD. But I'm afraid that global variables make me a little nervous, like time.time(), PRNG state, and PWD.

added 2 characters in body
Source Link
J_H
  • 42.3k
  • 3
  • 38
  • 157

Clearly it's not necessary, but it isis considered polite to introduce all attributes in the constructor. It givegives a maintenance engineer a heads-up about what to watch for. Consider defaulting to None, or maybe have a helper return a tuple, which an assignment can unpack.

The Thing inheritance for Bullet and Spaceship is very nice. Good use of super().!

Clearly it's not necessary, but it is considered polite to introduce all attributes in the constructor. It give a maintenance engineer a heads-up about what to watch for. Consider defaulting to None, or maybe have a helper return a tuple, which an assignment can unpack.

The Thing inheritance for Bullet and Spaceship is very nice. Good use of super().

Clearly it's not necessary, but it is considered polite to introduce all attributes in the constructor. It gives a maintenance engineer a heads-up about what to watch for. Consider defaulting to None, or maybe have a helper return a tuple, which an assignment can unpack.

The Thing inheritance for Bullet and Spaceship is very nice. Good use of super()!

Source Link
J_H
  • 42.3k
  • 3
  • 38
  • 157

The graphics are beautiful. I appreciate the attention to details, like the background stars.

assets directory

This certainly is not bad:

os.chdir(os.path.dirname(os.path.abspath(__file__)))

It's not like there was something else that was going to care about $PWD. But I'm afraid that global variables make me a little nervous, like time.time(), PRNG state, and PWD.

As an unrelated detail, the Path API tends to be more concise and convenient than the old os.path.* routines.

Standard idiom would start with

source_folder = Path(__file__).parent.resolve()

from which we might assign assets, and continue on with
assets / "rocket_stage1.png", etc.

manifest constants

You have a lot of them, and they all look great. Thank you for that.

nit: Delete # 50000 on MAX_AREA, as it's no longer needed. Consider spelling the value 30_000, so it's easier to read.

from ... import ...

import pygame

Clearly this works. But consider beefing it up with from pygame import Vector2 and similar, so we can make some repeated expressions a bit shorter.

gun positions

GUN_POSITIONS has lots of magic numbers such as 6 and 19, which is Fine. What is missing is a helpful github URL showing a blown up graphic, so we can understand how (6, 4) and (19, 0) fit into the graphic that end users will see.

At some point, we might wish to revise that graphic, perhaps because a Level 2 spacecraft has 50% more firepower sprouting from its wings. And perhaps the Art Director will approve the final design. Tying these coordinates to a specific creative image would help the process.

namedtuple

LEVEL_INFO = [
    (3, 3, 1),
    (3, 3, 1),
    (5, 5, 2),
    (5, 5, 2),
    (7, 5, 2),
]  # (bodies, rockets, turning_rockets)

Thank you for that very helpful comment.

As it stands, the first parameter to level_info_multiplier is cryptic and is in need of a MANIFEST_CONSTANT, but we have better approaches available.

Consider defining this:

from collections import namedtuple

Level = namedtuple("Level", "bodies, rockets, turning_rockets")

Then we don't need a comment.

And we can refer to .bodies rather than a cryptic ...[0] info subscript.

BTW, the clamp helper is concise and beautiful. Well, generally import_image, dir_dis_to_xy, and xy_to_dir_dis are all very nice helpers. In dir_dis_to_xy I guess pygame has an inconvenient "upside down" convention on Y coords that you have to roll with, sigh. In xy_to_dir_dis I confess I don't see any reason for negating before squaring; I just don't see that there's some story we're advancing there. Whether you negate or not, the square will be positive.

catalog of object attributes

    def rot_center(self, ...):
        self.rotated_image = ...

The __init__ constructor did not mention that attribute.

Clearly it's not necessary, but it is considered polite to introduce all attributes in the constructor. It give a maintenance engineer a heads-up about what to watch for. Consider defaulting to None, or maybe have a helper return a tuple, which an assignment can unpack.

            self.rect = ...
        ...
        self.mask = ...

Oh, dear! I see there's more to worry about.

singleton address

        if pos != None:

As a weird python community thing, please prefer if pos is not None:. Or use ruff or some other linter to help you with odd cases like that.

Also, here it would be simpler to test the positive case and deal with that first.

Better, unconditionally assign this:

    self.rect = self.rotated_image.get_rect(center=pos or self.pos)

The Thing inheritance for Bullet and Spaceship is very nice. Good use of super().

vector vs scalar

I found this slightly odd:

class Spaceship(Thing):
    def __init__( ... ):
        ...
        self.acceleration = 0
        self.velocity = pygame.Vector2(0)

We're simplifying, sure, that's cool. Just saying that I felt an acceleration would be a vector quantity, pointing in a similar direction to velocity, whatever.

In version 2, maybe spaceships operate near massive objects, where acceleration always points down the gravity well....

short functions

Let me just take a minute to complement you on your beautifully short, comprehensible methods. They do One thing, Well.

Also, helpers like clamp() make some calling code admirably concise.

modulo

In turn_to_angle I'm not crazy about this:

    while turn > 180:
        turn -= 360
    while turn < -180:
        turn += 360

That seems like what the % mod operator is for.

Also, I'm not happy with the post-condition; I feel it's on the sloppy side.

We would like to

        assert -180 <= turn < 180

Or, at least I think I have seen that [-180, 180) half-open interval as a standard convention.

But the < > inequalities don't align with <=.

Also, I like stars_gen, it is aesthetically pleasing.

PEP-8

class space:

I don't understand what's going on with that name. Call it Space, please.

sgn

        if pos.x > 0:
            scroll_surf.blit(self.space_image, pos - pygame.Vector2(screen_size.x, 0))
        else:
            scroll_surf.blit(self.space_image, pos + pygame.Vector2(screen_size.x, 0))

We have a ± there.

Using copysign(), it seems like we could collapse those lines down to one.

Similarly for the Y direction.

handle_events

In the space class, it turns out that handle_events is Too Long. How do we know? Try to read the signature and also the last line of the method, without vertically scrolling. Yup, that's right, you can't. It's Too Long. There are several opportunities to Extract Helper.

Also, the Magic Number 5 is slightly worrying. It wants a MANIFEST_CONSTANT name, please.

Here is another troubling magic number:

        spaceships[player].level != 4

getattr

                max_value = getattr(spaceships[player], ...
                to_add = getattr(spaceships[player], ...
                before = getattr(spaceships[player], ...

Not sure if this is quite a Code Smell. But it certainly does seem inconvenient.

There are so many solutions. Make those objects namedtuples, or dataclasses, or give them custom behavior.

Usually we resort to getattr() when there is a fixed Public API which we're not at liberty to change. But that's not your situation at all. Alter the underlying implementation, for the convenience of the caller. That's how excellent Public APIs are forged!

Similarly with the subsequent setattr( ... ).

break

def run_game():
    while True:
        if handle_events():
            break

That works, but I'm not crazy about it. Consider switching to

def run_game():
    while not handle_events():
        ...

imports up top

This troubles me:

if __name__ == "__main__":
    ...
    import things

Move the import to top-of-file, please. We're not saving anything by conditionally importing it.

Also, the code within this __main__ guard is entirely Too Long, and should appear within def main():, hopefully with several helpers.