I'll soon begin teaching a beginners' programming class. It's voluntary, so I thought I'd make it interesting by teaching Python programming and then introduce the kids to Pygame, so that they can make their own games. To try out Pygame (I've never used it before) and to see exactly how easy it is to make a game, I made a clone of Flappy Bird.

What do you think? Could this be made simpler/shorter? Is there anything in there that I shouldn't teach my students?

@Timo no problem :) Yes, go ahead and change it there. Also, you might wait a while for more answers, adjust your code and post a follow-up review if you want more remarks on the updated code.
–
timAug 29 '14 at 18:52

Nice, Once the students complete their projects, and if their code is working, tell them to create accounts and post them here. Good Luck.
–
BhathiyaDec 5 '14 at 11:34

4 Answers
4

You have docstrings for your functions and classes, which makes your code better than 95% of code submitted to Code Review.

The behaviour of the pipes is split into several pieces: (i) the PipePair class; (ii) the motion, drawing, and destruction logic in main; (iii) the scoring logic in main; (iv) the factory function random_pipe_pair. It would make the code easier to understand and maintain if you collected all the pipe logic into methods on the PipePair class.

Similarly, the behaviour of the bird is distributed among several places: (i) the local variables bird_y and steps_to_jump in main; (ii) the "calculate position of jumping bird" logic; (iii) the flapping animation logic; (iv) the get_frame_jump_height function. It would make the code easier to understand if you collected all the bird logic into methods on a Bird class.

The word "jumping" doesn't seem like a good description of the behaviour of the bird.

Name is_bird_collision make not sense English.

In the collision logic you're effectively testing for intersection of rectangular hit boxes. Pygame provides a Rect class with various collide methods that would make your code clearer, and would make it easier to do things like drawing the hit boxes to help with debugging.

You store the pipes in a list, but this is inefficient when it comes to remove a pipe: list.remove takes time proportional to the length of the list. You should use a set, or, since you know that pipes get created on the right and destroyed on the left, a collections.deque.

When you test for collisions, you store the collision results in a list and then test to see if True is an element of the list. Instead, you should use the built-in function any:

if any(p.collides_with(bird) for p in pipes):

(This has the additional advantage of short-circuiting: that is, stopping as soon as a collision has been detected, instead of going on to test the remaining pipes.)

You measure time in frames (for example, pipes move leftwards at a particular number of pixels per frame). This has the consequence that you cannot change the framerate without having to change many other parameters. It is more general to measure time in seconds: this makes it possible to vary the framerate.

(In this kind of simple game you can get away with measuring in frames, but in more sophisticated games you'll need to be able to vary the framerate and so it's worth practicing the necessary techniques.)

In commit 583c3e49 you broke the game by (i) removing the function random_pipe_pair without changing the caller; and (ii) changing the local variable surface to an attribute self.surface in some places but not others.

We all make commits in error from time to time, but there are four commits after this one, which suggests that you haven't been testing your code before committing it. This is a bad habit to get into!

BUT... As you commented, you use the parens mostly to allow breaking long lines (probably following PEP8) without the ugly \. I totally agree with that, so yeah, keep 'em! (In fact I didn't even know this was possible, so thanks for the lesson, teach'!)

Good points! About the parentheses: either I'm using them for line continuation so I don't have to use that ugly backslash :), or I'm using them for clarity -- I know the precedence of and and or, but 'explicit is better than implicit', as the Zen of Python says :)
–
TimoAug 29 '14 at 17:16

1

I don't actually know Python but found the question interesting. About removing the parenthesis from if a or (b and c): I would leave those in, simply because it makes it explicit and you don't need to know/remember the operator precedence. I've worked with about 20 languages now and operator precedence rules is something I got sick of: just add parenthesis and everybody knows how it's supposed to work.
–
DarkDustAug 29 '14 at 17:26

I agree that explicit is better than implicit, but this is a bit bordering paranoid coding for me. BUT, I agree with your other point about line breaking, and I updated my post
–
janosAug 29 '14 at 17:29

Good points, thanks. Let's see how that works out.. I'll commit to Github when I'm finished.
–
TimoAug 29 '14 at 17:10

Finally... after some trouble with git, I committed the changes. I'll update the question as well.
–
TimoAug 29 '14 at 18:41

2

It is against CR policy to edit the code, because it invalidates the review. Please roll back the edit. You are more than welcome to post the updated code as a follow-up question.
–
vnpAug 29 '14 at 18:55

I think that unless you're tracking the rects which have changed, display.update() is no better than display.flip(), though I suppose for a student it would be easier to read.

I'm curious as to why collisions are being checked after the view updates? I know that in an event loop situation the actual order of processes can be a little lax, especially if you're working at a high FPS, and I see that it presents the code in a way where the end of the loop contains the "exit or don't" code, but I guess I'm of the opinion that all the state checking work should happen before the view is updated. That's probably small potatoes, I've no idea the age or experience of the students you're working with.

I have to agree that the paused handling is a bit weird, just because it kind of starts a discussion about using states to control the flow of the (very short) script. Would rather see something more like "if not paused: do_stuff()" rather than "if paused: continue".

It's also sort of interesting which aspects of the PyGame API you've chosen to work with; for my part, it feels very strange to do an Intro To Games class without working with sprites. The Pygame sprite.Sprite object is a pretty helpful thing to have around! But as I look over the code it seems like it introduces a range of different concepts, so maybe there's no call for adding yet another concept to the mix.

Thanks! Because I'm breaking from the main loop directly if a collision is detected, I put it at the end when I first coded that part of the game so that the user isn't told that they crashed while the bird is still displayed a few pixels away from the pipe. I'll move all state checking to the beginning of the loop and just set done = True -- like it's done for the QUIT event, for example.
–
TimoAug 29 '14 at 18:17

Good point about display.flip() vs display.update() -- I must have overlooked flip when I read through the documentation.
–
TimoAug 29 '14 at 18:19

I'm not entirely sure what you mean by if not paused: do_stuff() -- are you suggesting that I extract the entire loop body into another function and just call it if the game isn't paused?
–
TimoAug 29 '14 at 18:21

Regarding sprites: I could either create a new Bird class subclassing sprite.Sprite and make PipePair subclass sprite.Group, which would, IMHO, make the game more complex than it needs to be; or I could just replace every usage of images[...] with some_sprite.image or something similar, and replace every display_surface.blit(some_pipe_pair.surface, ...) with some_pipe_pair.draw(display_surface), which wouldn't really simplify the code either, IMHO. But I would definitely introduce sprites somehow, perhaps in a different example.
–
TimoAug 29 '14 at 18:30

Well you don't necessarily have to pull all the view stuff out of the view, but I see what you're saying. As for subclassing sprite.Group, I may have misunderstood what you meant, but Group is a container for sprites with convenience methods. For example, MyGroup.draw(DispSurf) would blit all sprites in that group without the use of a for loop; and a call like pygame.sprite.spritecollide(Bird, PipesGroup) would return a list of all the sprites in PipesGroup the bird hit, so a simple "if spritecollide(Bird, PipesGroup):" would serve as hit detection (since an empty list is falsy)
–
StickAug 29 '14 at 19:14