10
\$\begingroup\$

Just looking for some pointers on how I could improve the code. This is my first game ever and python is my first language so it's bit messy but I'll be happy to explain stuff if needed. I'm kinda frustrated cause some bits look really ugly and I hope you guys can help me out. Here it is:

EDIT: cleaned up the code and added some new stuff.

objects.py

import pygame
import math
import random
from Vec2D import Vec2D
from constants import *
from pygame.locals import *

class Text(object):

    def __init__(self, value, size, color,
                 left_orientation=False,
                 font=None,
                 x=0, y=0,
                 top=None, bottom=None, left=None, right=None,
                 centerx=None, centery=None):

        self._size = size
        self._color = color
        self._value = value
        self._font = pygame.font.Font(font, self._size)
        self.width, self.height = self._font.size(self._value)
        self._left_orientation = left_orientation

        self.image = self._create_surface()
        self.rect = self.image.get_rect()
        if x: self.rect.x = x
        if y: self.rect.y = y
        if top: self.rect.top = top
        if bottom: self.rect.bottom = bottom
        if left: self.rect.left = left
        if right: self.rect.right = right
        if centerx: self.rect.centerx = centerx
        if centery: self.rect.centery = centery

    def _create_surface(self):
        return self._font.render(self._value, True, self._color)

    def set_value(self, new_value):
        if new_value != self._value:
            self._value = new_value
            self.image = self._create_surface()

            new_rect = self.image.get_rect(x = self.rect.x, y = self.rect.y)
            if self._left_orientation:
                width_diff = new_rect.width - self.rect.width
                new_rect.x = self.rect.x - width_diff
            self.rect = new_rect

class Ball(pygame.sprite.Sprite):

    def __init__(self, game, vector=Vec2D()):
        super(Ball, self).__init__()

        self.image = pygame.Surface((BALL_RADIUS*2, BALL_RADIUS*2))
        self.rect = self.image.get_rect()
        self._draw_ball()

        screen = pygame.display.get_surface()
        self.area = screen.get_rect().inflate(-GAP*2, 0)

        self.vector = vector
        self.game = game
        self.start_to_the = 'left'
        self.reinit()

    def _draw_ball(self):
        self.image.fill(BLACK)
        self.image.set_colorkey(BLACK, RLEACCEL)
        pygame.draw.circle(self.image, WHITE, (self.rect.centerx, self.rect.centery), BALL_RADIUS)

    def reinit(self):
        self.rect.centerx = self.area.centerx
        self.rect.centery = self.area.centery

        if self.start_to_the == 'left':
            self.vector = Vec2D(-BALL_SPEED, 0)
        else:
            self.vector = Vec2D(BALL_SPEED, 0)


    def update(self, dt):
        self.rect = self.calcnewpos(dt)
        self.handle_collision()

    def calcnewpos(self, dt):
        (dx, dy) = self.vector.get_xy()
        return self.rect.move(dx, dy)

    def handle_collision(self):
        (dx, dy) = self.vector.get_xy()

        if not self.area.contains(self.rect):
            if self._hit_topbottom():
                dy = -dy

            elif self._hit_leftright():
                side = self._hit_leftright()
                self.game.increase_score(side)

                if side == 'left':
                    self.start_to_the = 'right'
                elif side == 'right':
                    self.start_to_the = 'left'

                self.reinit()
                return
        else:
            if self.hit_paddle():
                paddle = self.hit_paddle()
                if paddle.side == 'left':
                    self.rect.left = GAP + PADDLE_WIDTH
                elif paddle.side == 'right': 
                    self.rect.right = SCREEN_WIDTH - (GAP + PADDLE_WIDTH)
                dx = -dx

                dy = (self.rect.centery - paddle.rect.centery)
                if dy <= -32:
                    dy = -32
                elif -32 < dy <= -16:
                    dy = -16
                elif -16 < dy < 16:
                    dy = 0
                elif 16 <= dy < 32:
                    dy = 16
                elif dy >= 32:
                    dy = 32
                dy /= 4
                paddle.collided = True

        self.vector = Vec2D(dx, dy)

    def _hit_topbottom(self):
        return self.rect.top < 0 or self.rect.bottom > SCREEN_HEIGHT

    def _hit_leftright(self):
        if self.rect.left < self.area.left: return 'left'
        elif self.rect.right > self.area.right: return 'right'

    def hit_paddle(self):
        player = self.game.player
        enemy = self.game.enemy
        paddles = [player, enemy]

        for paddle in paddles:
            if self.rect.colliderect(paddle.rect): return paddle

class Paddle(pygame.sprite.Sprite):

    def __init__(self):
        super(Paddle, self).__init__()

        self.image = pygame.Surface(PADDLE_SIZE)
        self.rect = self.image.get_rect()
        self._draw_paddle()

        screen = pygame.display.get_surface()
        self.area = screen.get_rect()

        self.collided = False

    def _draw_paddle(self):
        self.image.fill(WHITE)

    def reinit(self):
        self.state = 'still'
        self.movepos = [0, 0]
        self.rect.centery = self.area.centery

    def update(self):
        new_rect = self.rect.move(self.movepos)
        if self.area.contains(new_rect):
            self.rect = new_rect
        pygame.event.pump()

class Player(Paddle):

    def __init__(self, side):
        super(Player, self).__init__()
        self.side = side
        self.speed = PLAYER_SPEED
        self.score = 0
        self.reinit()

    def update(self, dt):
        keys = pygame.key.get_pressed()
        if keys[K_UP]:
            self.movepos[1] = -self.speed * dt
        if keys[K_DOWN]:
            self.movepos[1] = self.speed * dt
        super(Player, self).update()

    def reinit(self):
        super(Player, self).reinit()
        if self.side == 'left': self.rect.left = GAP
        elif self.side == 'right': self.rect.right = SCREEN_WIDTH - GAP

        self.score = 0

class Enemy(Paddle):

    def __init__(self, game):
        super(Enemy, self).__init__()
        self.game = game 
        self.speed = ENEMY_SPEED
        self.side = 'right' if PLAYER_SIDE == 'left' else 'left'
        self.hitpos = 0
        self.score = 0
        self.reinit()

    def update(self, dt):
        super(Enemy, self).update()

        ball = self.game.ball 
        hitspot_ypos = self.rect.centery + self.hitpos

        if (hitspot_ypos - ball.rect.centery) not in range(-5, 5):
            if hitspot_ypos > ball.rect.centery:
                self.movepos[1] = -self.speed * dt
            if hitspot_ypos < ball.rect.centery:
                self.movepos[1] = self.speed * dt
        else:
            self.movepos[1] = 0

        if self.collided:
            self.hitpos = random.randrange(-40, 40)
            self.collided = False

    def reinit(self):
        super(Enemy, self).reinit()

        if self.side == 'left': self.rect.left = GAP
        elif self.side == 'right': self.rect.right = SCREEN_WIDTH - GAP

        self.score = 0

game.py

#!python3

import pygame
import sys
import random
import math
from Vec2D import Vec2D
from constants import *
from pygame.locals import *
from objects import Text, Ball, Player, Enemy

class Game(object):

    def __init__(self):
        self.ball = Ball(self, Vec2D(random.choice([-BALL_SPEED, BALL_SPEED]), 0))
        self.enemy = Enemy(self)
        self.player = Player(PLAYER_SIDE)
        self.game_sprites = pygame.sprite.Group(self.ball, self.enemy, self.player)

        screen = pygame.display.get_surface()
        self.background = pygame.Surface(screen.get_size())

        self.reinit()

    def reinit(self):
        for sprite in self.game_sprites:
            sprite.reinit()

        self._draw_background()

        self.player_score = 0
        self.enemy_score = 0
        self.highest_score = 0
        self.winner = None

    def main(self):

        left_score = Text('0', 32, WHITE, True, right = SCREEN_WIDTH/2 - 20, top = 10)
        right_score = Text('0', 32, WHITE, left = SCREEN_WIDTH/2 + 20, top = 10)

        pause_text = Text('PAUSE', 64, RED, centerx = SCREEN_WIDTH/2, centery = SCREEN_HEIGHT/2)

        theme_music = pygame.mixer.music.load('theme.mp3')

        clock = pygame.time.Clock()
        paused = False

        self.countdown_animation()
        screen.blit(self.background, [0, 0])

        pygame.mixer.music.play(-1)

        while 1:
            dt = clock.tick(FPS) / 1000

            for event in pygame.event.get():
                if event.type == QUIT or (event.type == KEYUP and event.key == K_ESCAPE):
                    pygame.quit()
                    sys.exit()
                elif event.type == KEYUP:
                    if event.key == K_UP or event.key == K_DOWN:
                        self.player.movepos = [0, 0]
                        self.player.state = 'still'
                elif event.type == KEYDOWN:
                    if event.key == K_p:
                        if not paused:
                            pygame.mixer.music.pause()
                            paused = True
                        else:
                            pygame.mixer.music.unpause()
                            paused = False

            if not paused:

                self.game_sprites.clear(screen, self.background)

                screen.blit(self.background, left_score.rect, left_score.rect)
                screen.blit(self.background, right_score.rect, right_score.rect)
                screen.blit(self.background, pause_text.rect, pause_text.rect)

                self.game_sprites.update(dt)

                self.player_score = self.player.score
                self.enemy_score = self.enemy.score

                left_score.set_value(str(self.player.score))
                right_score.set_value(str(self.enemy.score))

                if self.player.side != 'left':
                    left_score.set_value(str(self.enemy.score))
                    right_score.set_value(str(self.player.score))

                self.game_sprites.draw(screen)

                screen.blit(left_score.image, left_score.rect)
                screen.blit(right_score.image, right_score.rect)

                self.highest_score = max(self.player_score, self.enemy_score)

                if self.highest_score == TOP_SCORE:
                    if self.player.score > self.enemy.score:
                        self.winner = 'player'
                    elif self.enemy.score > self.player.score:
                        self.winner = 'enemy'

                    pygame.mixer.music.stop()
                    self.game_won_animation()

                    self.reinit()

                    self.countdown_animation()
                    screen.blit(self.background, [0, 0])
                    pygame.mixer.music.play(-1)

            else:
                screen.blit(pause_text.image, pause_text.rect)

            pygame.display.flip()

    def countdown_animation(self):
        font = pygame.font.Font(None, 100)
        beep = pygame.mixer.Sound('beep1.wav')

        count = COUNTDOWN
        while count > 0:
            screen.fill(BLACK)

            font_size = font.size(str(count))
            # calculate text position so that its center = screen center
            textpos = [SCREEN_WIDTH/2 - font_size[0]/2, SCREEN_HEIGHT/2 - font_size[1]/2]

            screen.blit(font.render(str(count), True, WHITE, BGCOLOR), textpos)
            pygame.display.flip()

            beep.play()

            count -= 1

            pygame.time.delay(1000)

    def game_won_animation(self):

        screen.blit(self.background, self.ball.rect, self.ball.rect)

        if self.winner == 'player':
            message = 'You won!'
            endgame_sound = pygame.mixer.Sound('won.wav')
            color = BLUE
        elif self.winner == 'enemy':
            message = 'You suck!'
            endgame_sound = pygame.mixer.Sound('lost.wav')
            color = RED

        winner_text = Text(message, 128, color,
                           centerx = SCREEN_WIDTH/2, centery = SCREEN_HEIGHT/2)
        screen.blit(winner_text.image, winner_text.rect)
        pygame.display.flip()

        endgame_sound.play()

        pygame.time.delay(5000)
        screen.blit(self.background, winner_text.rect, winner_text.rect)

    def increase_score(self, side):
        if self.player.side == side:
            self.enemy.score += 1
            self.winner = self.enemy.side
        else:
            self.player.score += 1
            self.winner = self.player.side

    def _draw_background(self):
        self.background.fill(BGCOLOR)

        leftcolor = BLUE
        rightcolor = RED
        if self.player.side != 'left':
            leftcolor = RED
            rightcolor = BLUE

        # draw left line
        pygame.draw.line(self.background, leftcolor, (GAP, 0), (GAP, SCREEN_HEIGHT), 2)

        # draw right line
        pygame.draw.line(self.background, rightcolor,
                         (SCREEN_WIDTH - GAP, 0),
                         (SCREEN_WIDTH - GAP, SCREEN_HEIGHT), 2)

        # draw middle line
        pygame.draw.line(self.background, WHITE, 
                         (SCREEN_WIDTH/2, 0),
                         (SCREEN_WIDTH/2, SCREEN_HEIGHT), 2)

if __name__ == '__main__':
    pygame.init()
    screen = pygame.display.set_mode(SCREEN_SIZE)
    pygame.display.set_caption('Pong!')
    pong = Game()
    pong.main()

constants.py

SCREEN_WIDTH = 640
SCREEN_HEIGHT = 480
PADDLE_WIDTH = 20
PADDLE_HEIGHT = 80
BALL_SPEED = 5
BALL_RADIUS = 10
PLAYER_SPEED = 200
ENEMY_SPEED = 200
GAP = 40

#         R    G    B
BLACK = (  0,   0,   0)
WHITE = (255, 255, 255)
RED   = (255,   0,   0)
BLUE  = (  0,   0, 255)

SCREEN_SIZE = (SCREEN_WIDTH, SCREEN_HEIGHT) 
PADDLE_SIZE = (PADDLE_WIDTH, PADDLE_HEIGHT)
BGCOLOR = BLACK
PLAYER_SIDE = 'left'
TOP_SCORE = 10
COUNTDOWN = 3
FPS = 30

Vec2D.py

import math

class Vec2D(object):

    def __init__(self, x = 0., y = 0.):
        self.x = x
        self.y = y
        self.magnitude = self.get_magnitude()

    def __str__(self):
        return "%s, %s" %(self.x, self.y)

    @classmethod
    def from_points(cls, P1, P2):
        return cls(P2[0] - P1[0], P2[1] - P1[1])

    @classmethod
    def from_magn_and_angle(cls, magn, angle):
        x = magn * math.cos(angle)
        y = magn * math.sin(angle)
        return cls(x, y)

    def get_magnitude(self):
        return math.sqrt(self.x ** 2 + self.y ** 2)

    def get_xy(self):
        return (self.x, self.y)

Here is the full program.

\$\endgroup\$
  • 1
    \$\begingroup\$ There's no need for a try-except block to surround the imports. You probably don't want to run the script if imports fail, right? \$\endgroup\$ – Iulius Curt Sep 18 '13 at 21:23
  • \$\begingroup\$ This code isn't runnable as-is. Can you post the contents of the Vec2D and constants modules, please? \$\endgroup\$ – Gareth Rees Sep 19 '13 at 14:08
  • \$\begingroup\$ Use constants rather than hard coded numbers. Similarly for strings. \$\endgroup\$ – Vivek Jain Sep 19 '13 at 16:02
  • \$\begingroup\$ @GarethRees Sorry. Its all there now. \$\endgroup\$ – bzzr Sep 19 '13 at 22:23
13
\$\begingroup\$

1. First version

  1. I can't run it:

    >>> import game
    ImportError: No module named Vec2D
    

    There's no Vec2D package available from the Python Package Index. So where does this come from? I guess it must be your own vector package, but if so, you need to post it here.

    Similar remarks apply to the constants module.

  2. I don't know which version of Python to use to run this. Your parenthesized print statements, and your use of dt = clock.tick(FPS) / 1000 suggest that Python 3 is required, but on the other hand your use of super(Ball, self) instead of plain super() suggest that you are thinking about running on Python 2.

    It would be sensible to have a comment near the top noting the supported version(s) of Python.

    Alternatively, you might consider rewriting the code to be portable between Python 3 and Python 2, by changing

    print("ImportError:", message)
    

    to something like

    print("ImportError: {0}".format(message))
    

    and

    dt = clock.tick(FPS) / 1000
    

    to

    dt = clock.tick(FPS) / 1000.0
    
  3. You've surrounded your import statements with a try ... except that suppresses the ImportError resulting from a failed import. Why did you do this? If any of these modules can't be imported, the right thing to do is to fail immediately with an ImportError, not to carry on running and fail later with a NameError.

  4. You have variables named BLACK, WHITE, RED, BLUE and so on. This seems like a bad idea because it's not clear from the name what those variables are for (which things are coloured black?), and if you want to configure the colours then the names will end up looking silly:

    RED = pygame.Color('blue')
    

    Better to have names that refer to the purpose of the variable, for example ENEMY_COLOR instead of RED and PLAYER_COLOR instead of BLUE.

  5. You use private method names like __draw_ball and __hit_topbottom. Why do you do this? The intended use case is "letting subclasses override methods without breaking intraclass method calls" but that doesn't seem to apply to your code. All that private method names achieve in your case is to make the program slightly more difficult to debug:

    >>> ball.__hit_leftright()
    AttributeError: 'Ball' object has no attribute '__hit_leftright'
    >>> ball._Ball__hit_leftright()
    'left'
    

2. Second version

Thank you for providing copies of the constants and Vec2D modules.

2.1. Major problems

  1. It still doesn't work:

    Traceback (most recent call last):
      File "game.py", line 142, in <module>
        pong.main()
      File "game.py", line 38, in main
        self.theme_music = pygame.mixer.music.load('theme.mp3')
    pygame.error: Couldn't open 'theme.mp3'
    

2.2. Vec2D module

  1. There's no documentation: in particular, there are no docstrings. How are people expected to know how to use this module?

  2. There are lots of vector libraries already out there. Did you really need to write your own? If your Python installation was built with Tk, then there's one in the standard library:

    >>> from turtle import Vec2D
    >>> v = Vec2D(1, 2)
    >>> w = Vec2D(3, 4)
    >>> abs(w) # returns magnitude of the vector
    5.0
    >>> v + w
    (4.00,6.00)
    >>> v * 2
    (2.00,4.00)
    

    or if you need more features than turtle.Vec2D, the Python Package Index has several vector libraries (Daniel Pope's wasabi.geom might be suitable).

  3. When a Vec2D object is created, you set its magnitude:

    self.magnitude = self.get_magnitude()
    

    but your game never uses this property. Consider using the @property decorator instead so that the magnitude only gets computed when you need it:

    @property
    def magnitude(self):
        return math.hypot(self.x, self.y)
    

    (Also note the use of math.hypot from the standard library.)

  4. You have a class method from_magn_and_angle but you only ever call it with angle equal to zero, for example:

    self.vector = Vec2D.Vec2D.from_magn_and_angle(BALL_SPEED, 0)
    

    which is the same as:

    self.vector = Vec2D.Vec2D(BALL_SPEED, 0)
    

2.3. Constants

  1. It would make your code clearer if you used Pygame's colour names. Instead of

    BLACK = (  0,   0,   0)
    BGCOLOR = BLACK
    

    consider something like:

    BACKGROUND_COLOR = pygame.Color('black')
    
  2. There's no explanation of the meaning of the constants. We can guess from the name that SCREEN_HEIGHT = 480 gives the height of the screen in pixels, but what about GAP? It's the gap betwen something and something else, but what?

    Or consider BALL_SPEED = 5. It's probably the speed of the ball in some units. But what units? Pixels per second? Pixels per frame?

    From examining the code it seems that BALL_SPEED is in pixels per frame, but PLAYER_SPEED and ENEMY_SPEED are in pixels per second. This is particularly confusing! It also means that if you change the framerate, then the ball will speed up or slow down. Better to specify all speeds per second, so that you can easily change the framerate.

2.4. Text

  1. The Text._size member is only used to create the font, so there is no need to store it in the instance.

  2. Why not make Text a subclass of pygame.sprite.Sprite so that you can draw it using a sprite group?

  3. Lots of code is repeated betwen Text.__init__ and Text.set_value. Why not have the former call the latter?

  4. The Text.__init__ constructor takes many keyword arguments, which you then apply to self.rect like this:

    if top: self.rect.top = top
    ...
    

    The first problem is that the test if top: means that you can't set top to zero. You should write something like this:

    if top is not None: self.rect.top = top
    

    But it would be much simpler to use Python's ** keyword argument mechanism to take any number of keyword arguments, and pass them all to Surface.get_rect. Like this:

    class Text(pygame.sprite.Sprite):
        def __init__(self, text, size, color, font=None, **kwargs):
            super(Text, self).__init__()
            self.color = color
            self.font = pygame.font.Font(font, size)
            self.kwargs = kwargs
            self.set(text)
    
        def set(self, text):
            self.image = self.font.render(str(text), 1, self.color)
            self.rect = self.image.get_rect(**self.kwargs)
    

2.5. Ball class

  1. The Ball class sets a collided flag on a paddle when it collides with it. The purpose of this flag is to communicate to the Enemy class, telling it to select a new value for hitpos the next time that the update method is called. This does not seem very elegant! Why not have a method instead, that can be subclassed? For example, in the Ball class you'd replace:

    paddle.collided = True
    

    with

    paddle.collided()
    

    and then in the Paddle class you'd have a base method (that does nothing):

    def collided(self):
        "Handle collision with ball."
        pass
    

    which you'd override in the Enemy subclass:

    def collided(self):
        self.hitpos = random.randrange(-40, 40)
    
  2. Your use of black as the transparent colorkey means that you cannot configure the game to have a black ball. An alternative approach would be to use per-pixel alpha, and then you could omit the lines:

    self.image.fill(BLACK)
    self.image.set_colorkey(BLACK, RLEACCEL)
    
  3. Since _draw_ball is called only from Ball.__init__ and is really short (just one line after making the above change), why not inline it there? Similarly for _draw_paddle.

  4. There's no point in passing a vector argument to the Ball constructor because it will be immediately overwritten by the call to reinit.

  5. The name vector doesn't tell you what the purpose of the vector is: I suggest velocity instead.

  6. The calcnewpos and handle_collision methods are only called from one place in update, so I suggest inlining them there. Similarly, the _hit_leftright and _hit_topbottom methods are only called from one place in handle_collision.

  7. It's possible for the ball to hits the paddle and the edge of the playing area at the same time, but this case is not handled.

  8. If you're going to call _hit_topbottom and _hit_leftright, why also call self.area.contains?

  9. _hit_topbottom is implemented like this:

    return self.rect.top < 0 or self.rect.bottom > SCREEN_HEIGHT
    

    but shouldn't it be

    return self.rect.top < self.area.top or self.rect.bottom > self.area.bottom
    

    so that you can adjust the playing area if you need to?

  10. This code

    if dy <= -32:
        dy = -32
    elif -32 < dy <= -16:
        dy = -16
    elif -16 < dy < 16:
        dy = 0
    elif 16 <= dy < 32:
        dy = 16
    elif dy >= 32:
        dy = 32
    

    looks suspicious because there's no else: clause at the end of the chain of if: ... elif: ... statements. Also, these could be combined into one line, perhaps like this:

    dy = math.copysign(min(abs(dy) // 16 * 16, 32), dy)
    

    It also seems very unsatisfactory to me that the ball can only travel at five angles. Wouldn't the game be more interesting (and require more skill to play) if the ball had a wider range of behaviour? You could try something like this:

    dy = math.copysign(min(abs(dy), 32), dy)
    

2.6. Paddle, Player, and Enemy classes

  1. There seems to be a lot of shared code between the Player and Enemy classes. For example, both classes have the following setup in their __init__ methods:

    self.ball = ball
    self.side = ???
    self.speed = ???
    self.score = 0
    self.reinit()
    

    Why not put this shared code into Paddle.__init__?

  2. Similarly, Player.reinit and Enemy.reinit are identical. Why not put this shared code into Paddle.reinit?

  3. Code like this looks suspicious:

    if self.side == 'left': self.rect.left = GAP
    elif self.side == 'right': self.rect.right = SCREEN_WIDTH - GAP
    

    because there's no else: clause. What happens if self.side is neither 'left' or 'right'? You could raise an error:

    if self.side == 'left': self.rect.left = GAP
    elif self.side == 'right': self.rect.right = SCREEN_WIDTH - GAP
    else:
        raise ValueError("self.side = '{0}': must be 'left' or 'right'"
                         .format(self.side))
    

    Or you could assert that self.side is correct:

    assert(self.side in {'left', 'right'})
    if self.side == 'left': self.rect.left = GAP
    else: self.rect.right = SCREEN_WIDTH - GAP
    

    But really I think that this is eveidence that the "side" mechanism needs rethinking. I would consider specifying self.rect directly instead of self.side.

  4. Player.update has code for updating self.hitpos, but this is never used.

  5. This line of code:

     if (spot - self.ball.rect.centery) not in range(-5, 5):
    

    has a couple of problems. First, it only works so long as spot and self.ball.rect.centery are integers. Should you ever change the code so that either is floating-point, then this code would break. Second, each time you visit this line you construct a new range object which you then throw away. Third, range(-5, 5) only goes up to 4, which might not be what you meant. Instead, write something like:

    if abs(spot - self.ball.rect.centery) > 5:
    
  6. In code like this:

    if spot > self.ball.rect.centery:
        self.movepos[1] = -self.speed * dt
    if spot < self.ball.rect.centery:
        self.movepos[1] = self.speed * dt
    

    you could make use of math.copysign:

    self.movepos[1] = math.copysign(self.speed * dt, self.ball.rect.centery - spot)
    

    but see below for a different suggestion.

  7. The remaining code in the update methods of the Player and Enemy classes, consists of determining which direction to move (up or down?) and then calling the superclass method. It would be simpler to turn this around and have a method (move, say) which determines the direction to move.

    So Paddle.update would look like this:

    def update(self, dt):
        new_rect = self.rect.move([0, self.move(dt)])
        if self.area.contains(new_rect):
            self.rect = new_rect
        pygame.event.pump()
    

    and then the Player class would look like this:

    class Player(Paddle):
        def move(self, dt):
            keys = pygame.key.get_pressed()
            if keys[K_UP]: return -self.speed * dt
            elif keys[K_DOWN]: return  self.speed * dt
            else: return 0
    

    and the Enemy class like this:

    class Enemy(Paddle):
        def reinit(self):
            super().reinit()
            self.hitpos = 0
    
        def move(self, dt):
            dy = self.rect.centery + self.hitpos - self.ball.rect.centery
            if abs(dy) <= 5: return 0
            return math.copysign(self.speed * dt, dy)
    
        def collided(self):
            self.hitpos = random.randrange(-40, 40)
    
  8. The Player.movepos member is only used to transfer a value from Player.move to Player.update. When the return value is use instead, this member has no further use and can be removed.

  9. The Enemy behaviour results in jittery motion because it can only move full speed, so if it is trying to track a ball whose vertical component of velocity is less than enemy.speed * dt then it will alternate between frames of motion and frames of no motion. It would be yield a more pleasing result if the speed tapered off gracefully:

    def move(self, dt):
        dy = self.ball.rect.centery - self.rect.centery + self.hitpos 
        return math.copysign(min(abs(dy), self.speed * dt), dy)
    
  10. There seems to be no use for the Player.state variable.

  11. Instead of

    (self.rect.centerx, self.rect.centery)
    

    you can write

    self.rect.center
    

    And instead of

    self.rect.centerx = self.area.centerx
    self.rect.centery = self.area.centery
    

    you can write

    self.rect.center = self.area.center
    
  12. What is the purpose of calling pygame.event.pump in Paddle.update? The documentation says, "This function is not necessary if your program is consistently processing events on the queue through the other pygame.event functions."

2.7. Game class

  1. It's conventional to write

    while True:
    

    instead of

    while 1:
    
  2. Comments that merely document what happens in the next line are pretty useless:

    # initialize clock
    clock = pygame.time.Clock()
    

    Any reader could guess, from its capitalized name, that Clock is a constructor; and if they want to know what it does then they can read the documentation. It's generally best to use comments to explain why you're doing something, not what you're doing (if that's obvious from reading the code).

  3. You have a complicated drawing operation where you erase all the sprites and text at their old positions, update their positions, and then draw them at their new positions. This approach works here where you have a static unchanging background, but it won't work at all when the background moves or animates in any way.

    At the moment your main loop looks like this:

    # erase sprites
    screen.blit(self.background, self.ball.rect, self.ball.rect)
    screen.blit(self.background, self.player.rect, self.player.rect)
    screen.blit(self.background, self.enemy.rect, self.enemy.rect)
    
    # erase scores
    screen.blit(self.background, self.left_score.rect, self.left_score.rect)
    screen.blit(self.background, self.right_score.rect, self.right_score.rect)
    
    # erase pause text
    screen.blit(self.background, self.pause_text.rect, self.pause_text.rect)
    
    # update ...
    
    # draw sprites ...
    
    # draw scores ...
    

    But if you just draw the whole screen every frame, there will no longer be any need to erase the sprites, and the main loop will look something like this:

    # update ...
    
    # draw background
    screen.blit(self.background, screen.get_rect())
    
    # draw sprites ...
    
    # draw scores ...
    
  4. When the game is paused, you arrange not to draw the screen. But that means that there is no flip, and so no wait for the next frame. You compensate for this by adding a delay when the game is paused:

    pygame.time.delay(150)
    

    But if you just draw the frame normally when the game is paused, then there will be a flip as usual, and you won't need these delay calls.

3. Third version

It's not fair to ask us to review a moving target like this. It's best to finish writing your code before you ask people to review it.

Tom Gilb and Dorothy Graham, in their book Software Inspection, have a section (§4.4) about entry criteria for the inspection process. If a product is inspected at too early a stage, then there is a risk of wasting inspection effect:

checkers will find a large number of defects which can be found very easily by one individual alone, even the author. Inspections are cost-effective when the combined skills of a number of checkers are concentrated on a [program] which seems superficially all right, but actually has a number of major defects in it which will only be found by the intensive Inspection process, not by the author checking the product.

So in a professional context, it's important not to start inspection until the author has made the product as good as possible. (Here at Code Review we are a bit more relaxed.)

\$\endgroup\$
  • \$\begingroup\$ A little thing. parenthized print doesn't necessarily mean Python3. Try print("abcd") in Python2. It will work. \$\endgroup\$ – Aseem Bansal Sep 20 '13 at 5:04
  • \$\begingroup\$ @AseemBansal: The OP's code originally said print("ImportError:", message). In Python 2 this prints a tuple, which doesn't seem likely to be what the OP intended. \$\endgroup\$ – Gareth Rees Sep 20 '13 at 9:57
  • \$\begingroup\$ What do you mean by 'The code will become much simpler if you just draw the whole screen every frame.' in 2.7.3? \$\endgroup\$ – bzzr Sep 25 '13 at 23:23
  • \$\begingroup\$ @Jovito: see revised answer. \$\endgroup\$ – Gareth Rees Sep 26 '13 at 12:07
  • 1
    \$\begingroup\$ @Jovito: See Wikipedia for the meaning of inline expansion ("inlining" for short). But all I'm saying here is that if a method is called from only one place and doesn't have obvious "standalone" functionality, then it may not be worth making it into a separate method in the first place. But this is a matter of personal taste and judgement. \$\endgroup\$ – Gareth Rees Sep 26 '13 at 14:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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