HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonModerate

Pong with Pygame

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withpygamepong

Problem

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=Vec2

Solution


  1. First version



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

-
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


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

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

-
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'


  1. Second version



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

2.1. Major problems

-
It still doesn't work:

Traceback (most recent call last):
  File "game.py", line 142, in 
    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

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

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

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

-
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

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


-
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

-
The Text._size member

Code Snippets

>>> import game
ImportError: No module named Vec2D
print("ImportError:", message)
print("ImportError: {0}".format(message))
dt = clock.tick(FPS) / 1000
dt = clock.tick(FPS) / 1000.0

Context

StackExchange Code Review Q#31408, answer score: 12

Revisions (0)

No revisions yet.