Pong with Pygame

Question

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.

• Iulius Curt: 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?
• Gareth Rees: This code isn't runnable as-is. Can you post the contents of the Vec2D and constants modules, please?
• Vivek Jain: Use constants rather than hard coded numbers. Similarly for strings.
• bzrr: @GarethRees Sorry. Its all there now.

Answer 1

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.)

• Aseem Bansal: A little thing. parenthized print doesn't necessarily mean Python3. Try print("abcd") in Python2. It will work.
• Gareth Rees: @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.
• bzrr: What do you mean by 'The code will become much simpler if you just draw the whole screen every frame.' in 2.7.3?
• Gareth Rees: @Jovito: see revised answer.
• Gareth Rees: @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.