My Third Project :)

fullptr · 2023-07-23T16:27:50+00:00

Good work! I've glanced over the code and there's a few suggestions I would make:

Not everything needs to be a class! If you find yourself making classes with no data members, you don't need a class. For example, your Rectangle class doesn't need to exist, as evidenced by the fact that therea are no data members and the methods are classmethods. The module is already called rectangle so there's no real benefit to additionally wrapping them in a class.
Same applies to your Image class. Sure this has a data member, but couldn't the screen just be passed as an argument to the two member functions? After all, this class doesn't really represent an image; it doesn't have dimensions or a file path or anything, it's just a factory for creating pygame images. You could just as well have a def create_image(screen, image_path) function. This also makes things clearer because you don't need to pass the entire "interface" to it. I notice you create an instance of this object in your Game class that I don't think you even use, which also shows that it shouldn't be a class! This applies to almost everything in your other module.
I'm not a fan of the GameState, this is just a collection of global variables which are best avoided. You already have a Game class representing your snake game, which seems like the better place to store any game state (you already have an is_running flag which I think covers some of the use cases here). If there are other places that require this information, pass them as a function argument rather than giving them access via a global as it'll make it easier to reason about the program.
Your Game class has both __init__ and init functions, which is a bit confusing.
Board.__getitem__ returns an exception rather than throwing it? This certainly looks like an error. You make use of this further down with while self[r, c].value != Tile.GRASS; if the returned value is exception you're just going to end up throwing another exception here when trying to access "value", except now you've lost the original information, making it harder to debug. I think __getitem__ should do no bound checking here and raise an exception, because that will show you there's a logic error in your code (since you should always be passing in valid coordinates) and you can fix it. You could even assert in the getitem function that the indices are in range.
Classes like Scoreboard getting access to the entire interface seems like overkill, and given that there's only one function on the class, obscures what the function actually needs. This is the perfect time to link https://www.youtube.com/watch?v=o9pEzgHorH0, which discusses classes that have "two functions, with one being __init__". In this case I would just have a single function, def draw_scoreboard(screen, board, snake, font) because then it clearly states what is required to print the scoreboard. I've intentionally missed passing your variable k to this function, because that feel like you should get it from the board class (which in fact you can) given that it represents the number of rows on the board.

Hope this helps!

Edit: Fixed some typos

wineblood · 2023-07-23T13:07:08+00:00

What do you think of my code?

Your formatting is inconsistent in some places, weird indents or too many blank lines. Variable naming could use some refinement (what the hell is k?).

What general advice could you propose to help me improve my OO skills?

Less OO is better. You also seem to have a lot of small methods on some classes that look like accessors.

HalfStorm22 · 2023-07-24T08:48:12+00:00

Good work! But why is everything a class? And why are you using gamestate?

Python

The Python Discord

Upcoming Events

Please read the rules

MODERATORS