Passing two player objects to a class

I am a PyGame newbie and am learning as I go. I would like to become familiar with normal PyGame conventions. I would like you PyGame experts go over my code and let me know if there are any places I should modify my code to follow best practice.

The point of the game is to collect as many pills as possible. Yellows are worth 10, reds 20, blue 30 and black 40. First one to reach 15,000 wins. The ships are controlled using WASD and ↑↓←→.

Some areas of concern I am looking at are the following:

Where I created two separate classes to store the score information. I feel that I could do the same job with one class, but am a bit confused on how that would look since I am using a TextGroup and it needs to be passed both Ship objects on the call to TextGroup.update().

```
class Text(Entity):
def __init__(self, text, size, color, position, font=None):
Entity.__init__(self)
self.color = color
self.position = position
self.font = pygame.font.Font(font, size)
self.text = text
self.image = self.font.render(str(text), 1, self.color)
self.rect = self.image.get_rect()
self.rect.move_ip(position[0]-self.rect.width/2, position[1])

class Mass_Left(Text):
def __init__(self, text, size, color, position, font=None):
Text.__init__(self, text, size, color, position, font=None)

def update(self, ship_left, ship_right):
self.text = "mass: " + str(ship_left.density-169)
self.image = self.font.render(str(self.text), 1, self.color)
self.rect = self.image.get_rect()
self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])

class Mass_Right(Text):
def __init__(self, text, size, color, position, font=None):
Text.__init__(self, text, size, color, position, font=None)

def update(self, ship_left, ship_right):
self.text = "mass: " + str(ship_right.density-169)
self.image = self.font.render(str(self.text), 1, self.color)

Use OOP more appropriately

• Your Entity class adds absolutely nothing to pygame.sprite.Sprite so you’d be better of using the latter as base class for your Text, Ship and Pill ones.

• It is usually advised to use super to call the parent class constructor instead of explicitly naming the class. However, pygame documentation uses explicit pygame.sprite.Sprite.__init__(self) call so I’ll stick with it.

-
You’re incorrectly calling the Text.__init__ method in Mass_Left and Mass_Right: you force the font parameter to be None instead of using the value provided in __init__’s parameters. You should call super(Mass_Right, self).__init__(text, size, color, position, font=font). Or even remove the font= part. Renaming the parameter might help to understand: you’re doing

def __init__(self, text, size, color, position, f=None):
    Text.__init__(self, text, size, color, position, font=None)

instead of

def __init__(self, text, size, color, position, f=None):
    Text.__init__(self, text, size, color, position, font=f)

-
Your update method is pretty much the same between Mass_Left and Mass_Right. In fact it is the same if you are able to discriminate between ship_left and ship_right. Two possibilities: either you store a boolean value in the constructor telling which ship to use

class Text(pygame.sprite.Sprite):
    def __init__(self, text, size, color, position, left_side, font=None):
        pygame.sprite.Sprite.__init__(self)
        ...
        self.left_side = left_side

    def update(self, ship_left, ship_right):
        ship = ship_left if self.left_side else ship_right
        text = "mass: " + str(ship.density-169)
        self.image = self.font.render(str(self.text), 1, self.color)
        self.rect = self.image.get_rect()
        self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])

or you make better use of inheritance

class Text(pygame.sprite.Sprite):
    def __init__(self, text, size, color, position, font=None):
        pygame.sprite.Sprite.__init__(self)
        ...

    def _update(self, ship):
        text = "mass: " + str(ship.density-169)
        self.image = self.font.render(str(self.text), 1, self.color)
        self.rect = self.image.get_rect()
        self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])

class Mass_Left(Text):
    def __init__(self, text, size, color, position, font=None):
        super(Mass_Left, self).__init__(text, size, color, position, font)

    def update(self, ship_left, ship_right):
        self._update(ship_left)

class Mass_Right(Text):
    def __init__(self, text, size, color, position, font=None):
        super(Mass_Right, self).__init__(text, size, color, position, font)

    def update(self, ship_left, ship_right):
        self._update(ship_right)

Use only what you need

• Since your Texts subclasses directly convert the text into a surface, you don't need to store it. Better, you even don't need to pass it to the constructor since it is computed at each update.

-
In genRandom you can pass values to the tuple constructor instead of building a string and parsing it as a Python data-structure:

tup = (random.randrange(0, (WIN_W/2) - PILL_WIDTH), int(random.choice('1111111111111111111122222334')))

is the same as your

tup = literal_eval(stup)

without the need to build a string first. In fact, since the function is now only

def genRandom(size):
     xval_density = []
     for _ in range(size):
         xval_density.append((
             random.randrange(0, (WIN_W/2) - PILL_WIDTH),
             int(random.choice('1111111111111111111122222334'))))
     return xval_density

you could simplify further using a list-comprehension:

def genRandom(size):
    return [
        (random.randrange(0, (WIN_W/2) - PILL_WIDTH),
        int(random.choice('1111111111111111111122222334')))
        for _ in range(size)]

-
In loseGame you check for a bunch of conditions and then, detail which condition it actualy is. So you’re doing twice the work. Remove the outer if:

def loseGame(left, right):
    left.win = left.density > 1500
    right.win = right.density > 1500

    return not (left.win or right.win)

This one is also weird because its name and what it does are kind of opposite. I'd rename it is_game_ended, remove the not in the return statement and call it usinig while not game_ended: ...; game_ended = is_game_ended(ship_left, ship_right); ....

-
In moveShip you check for a bunch of conditions and then, detail which condition it actualy is. So you’re doing twice the work.

if self.player == 'right':
    if key[pygame.K_UP]:
        self.rect.y -= self.speed
    ...

Improve reusability

Your whole code has been built upon the assumption that there will be two players. But what if you want to add support for a third on a forth? Or add a single player mode agains an AI? Will you continue to a

StackExchange Code Review Q#111530, answer score: 2