"Pass the Pigs" game

Question

I was wondering if anyone could give a critique of my code. We are supposed to be using inheritance and OOP principles. All the functions but the PlayerSet class were given, so really I just need someone to critique that class.

The full code is here.

class PlayerSet:
    '''A group of players to play
    the Pass The Pigs game.'''

    def __init__(self,playerCount):
        '''PlayerSet(int) -> PlayerSet
        Contstructs a group of players,
        given how many there are and asks the
        name of each player.'''
        self.playerCount = playerCount # initialize count of players attribute
        self.playersList = [] # initialize list of players attrbute
        for i in range(1,self.playerCount+1): # for every i between 1 and plyerCount
            name = input('Player ' + str(i) + ', please enter your name:') # ask player i's name
            player = Player(name) # create a player with that name 
            self.playersList.append(player) # add the player to the list of players
        self.currentPlayer = self.playersList[0] # initialize current player attribute

    def __str__(self):
        '''print(PlayerSet) -> str
        Returns all of the player's
        name and score.'''
        string='' # will contain output
        for player in self.playersList: # for every player in the list of players 
            string+= player.get_name() + ' has ' + str(player.get_score()) + ' points \n' # add a string saying the player's name and current score
        return string # return the string

    def get_curr_name(self):
        '''PlayerSet.get_curr_name() -> str
        Returns the name of the current player.'''
        return self.currentPlayer.get_name() # return the name of the player

    def go_to_next_player(self):
        '''PlayerSet.go_to_next_player()
        Moves the current player to the next
        player.'''
        index = 0 # will contain index of the current player 
        for i in range(len(self.playersList)): # for every index in list of players
            if self.playersList[i] == self.currentPlayer: # if that index is correct
                index = i # assign index to the desired index

        index += 1 # add 1 to the desired index, we want to move to next player
        index %= self.playerCount # if the number of the player goes over, take the residue mod playerCount
        self.currentPlayer=self.playersList[index] # make the current player equal to the new index of the list of players 

    def is_curr_winner(self):
        '''PlayerSet.is_curr_winner() -> bool
        Returns if the current player has won.'''
        if self.currentPlayer.get_score() >= 100: # if the current player has won
            return True 
        else: # otherwise
            return False

    def take_turn(self):
        '''PlayerSet.take_turn()
        Takes the player's turn.'''
        self.currentPlayer.play_turn() # use player's method - play_turn() which plays a turn

    def play_game(self):
        '''PlayerSet.play_game()
        Plays the real Pass The Pigs game! Woohoo!'''
        while self.is_curr_winner() == False: # until a player has won
            print(self) # print the status of the players
            print(self.get_curr_name() + ", it's your turn") # print whose turn it is
            self.take_turn() # take the player's turn
            if self.is_curr_winner() == True: # if the current player has won
                print(self) # print the status of the players
                return self.get_curr_name() + ' has won!' # return which player has won
            self.go_to_next_player() # go to the next player

Answer 1

1. The class could be better named. PlayerSet is not just a set of players: it also knows how to play the game. I would consider calling it PassThePigs since it implements the logic for the game.

2. Most of your comments are unnecessary. Now, I understand that you're learning to program so you probably find it helpful at this stage to explain each line of code you write. But as you get more experienced, you'll realise that it's not necessary to write comments that merely say the same thing as the code. In cases like this:

   return self.currentPlayer.get_name() # return the name of the player
   

   you should find that you can easily read the code:

   return self.currentPlayer.get_name()
   

   as meaning "return the name of the current player".

   Comments like return the name of the player eventually become a burden rather than a help, because you have to remember to update the comment every time you update the code. For example, it would be all too easy to end up with code like this:

   return self.currentPlayer.get_score() # return the name of the player
   

   where the code has been changed and the comment is now wrong.

   The best comments explain things that can't easily be figured out by reading the code, for example why you wrote the code in a particular way. Here are a couple of my comments for comparison:

   From here:

   assert(limit == 10 ** max_digits) # algorithm works for powers of 10 only
   

   And from here:

   dt = self.clock.tick(FPS) / 1000.0 # convert to seconds
   

3. The Python style guide (PEP 8) suggests that you

   Limit all lines to a maximum of 79 characters [... This] makes it possible to have several files open side-by-side, and works well when using code review tools that present the two versions in adjacent columns.

   Also, shorter lines wouldn't disappear off the right hand side of code blocks here on Code Review.

   You're not obliged to follow PEP 8, but it makes it easier for you to collaborate with other Python programmers.

4. There's no need to store the number of players in self.playersCount because you can always get the number of players as len(self.playersList). Storing a fact in two places leads to a risk of inconsistency.

5. Instead of:

   'Player ' + str(i) + ', please enter your name:'
   

   consider:

   'Player {}, please enter your name:'.format(i)
   

6. Building a string by repeatedly appending to it is an anti-pattern in Python: it takes time that's quadratic in the number of append operations, because each += operation creates a new string. It's nearly always better to use the str.join method. So this code:

   string=''
   for player in self.playersList:
       string+= player.get_name() + ' has ' + str(player.get_score()) + ' points \n'
   return string
   

   is better written:

   return ''.join('{} has {} points\n'.format(player.get_name(), player.get_score())
                  for player in self.playersList)
   

7. Python does not generally need accessor methods like the get_name and get_score in your Player class. Instead of player.get_name(), consider using just player.name. That allows you to write:

   return ''.join('{0.name} has {0.score} points\n'.format(player)
                  for player in self.playersList)
   

8. If the player has exactly 1 point, then this is going to produce ungrammatical output:

   Parminder has 1 points
   

   You need something like:

   '{0.name} has {0.score} point{1}\n'.format(player, '' if player.score == 1 else 's')
   

9. The purpose of the __str__ method is "to compute the “informal” or nicely printable string representation of an object". But you are using it here to format the players' current scores. It would be better to use a method name like format_scores.

10. The go_to_next_player method seems very complicated. First, you have a loop where you try to find the index of the current player in the list:

    index = 0
    for i in range(len(self.playersList)):
        if self.playersList[i] == self.currentPlayer:
            index = i
    

    Whenever you find yourself iterating over the indexes of a list, consider using the enumerate function:

    index = 0
    for i, player in enumerate(self.playersList):
        if player == self.currentPlayer:
            index = i
    

    Second, when you find the player in the list, you set index but you carry on looping. It would be better to break out of the loop as soon as you find what you are looking for:

    index = 0
    for i, player in enumerate(self.playersList):
        if player == self.currentPlayer:
            index = i
            break
    

    I've shown you how to rewrite the loop, but in fact no loop is needed. Python comes with a built-in method list.index for this operation:

    index = self.playersList.index(self.currentPlayer)
    

    Finally, why go to this effect of looking up the current player in the list of players, when you could just remember the index instead of the player? In __init__ you could write:

    self.current_player_index = 0
    

    combined with a property:

    @property
    def currentPlayer(self):
        return self.playersList[self.current_player_index]
    

    and then the go_to_next_player method would be very simple:

    self.current_player_index = (self.current_player_index + 1) % len(self.playersList)
    

    A more "advanced" approach would be to use itertools.cycle. In __init__ you'd write:

    self.player_cycle = itertools.cycle(self.playersList)
    self.currentPlayer = next(self.player_cycle)
    

    and then the go_to_next_player method would become:

    self.currentPlayer = next(self.player_cycle)
    

11. This code:

    if self.currentPlayer.get_score() >= 100:
        return True
    else:
        return False
    

    can be simplified to:

    return self.currentPlayer.get_score() >= 100
    

    since Boolean expressions like a >= b evaluate to True or False.

12. Instead of:

    while self.is_curr_winner() == False:
    

    you could write:

    while not self.is_curr_winner():
    

    but actually this should be

    while True:
    

    since the winner detection has already been done (after the player took their turn).

13. The play_game method returns a string but this is not mentioned in the documentation for the method. (In fact, it's probably a mistake.)

14. Consider giving a name to the value 100 so that it's clear what it means and so that it's easy to find and change if you need to.

15. When you have a method that's called from only one place, and is only one line long, consider whether it's worth having a method at all. Adding methods is good because it improves the clarity of the code (you can give the method a name that explains its purpose, and provide documentation), but on the other hand each method increases the length of the code and makes it harder to read.

16. Once you do this inlining process, it will become clear that there isn't really any persistent state in the game: everything important happens inside PlayerSet.play_game. So is it really worth having a class at all? Compare your implementation with this one:

    from itertools import cycle
    
    def pass_the_pigs(self, n, winning_score=100):
        """Play a game of Pass the Pigs with n players."""
        players = [Player(input('Player {}, please enter your name:'.format(i)))
                   for i in range(1, n + 1)]
        for player in cycle(players):
            for p in players:
                print('{0.name} has {0.score} point{1}'
                      .format(p, '' if p.score == 1 else 's'))
            for p in players:
                if p.score >= winning_score:
                    print('{0.name} has won!'.format(p))
                    return
            print("{0.name}, it's your turn".format(player))
            player.play_turn()
    

    (Note that there's almost nothing here that's specific to Pass the Pigs — the same code would work as the main loop for any turn-based game where the players do not interact with each other, and where there's a winning score.)

Answer 2

General

Most of your comments are not really useful : comments should tell you why, not how.

Constructor

You don't need to keep the number of player if you keep the list of players anyway.

def __init__(self,playerCount):
    '''PlayerSet(int) -> PlayerSet
    Contstructs a group of players,
    given how many there are and asks the
    name of each player.'''
    self.playerCount = playerCount # initialize count of players attribute
    self.playersList = [] # initialize list of players attrbute
    for i in range(1,self.playerCount+1): # for every i between 1 and plyerCount
        name = input('Player ' + str(i) + ', please enter your name:') # ask player i's name
        player = Player(name) # create a player with that name 
        self.playersList.append(player) # add the player to the list of players
    self.currentPlayer = self.playersList[0] # initialize current player attribute

can become :

def __init__(self,playerCount):
    '''PlayerSet(int) -> PlayerSet
    Constructs a group of players,
    given how many there are and asks the
    name of each player.'''
    self.playersList = []
    for i in range(self.playerCount):
        name = input('Player ' + str(i+1) + ', please enter your name:')
        player = Player(name)
        self.playersList.append(player)
    self.currentPlayer = self.playersList[0]

which can become

def __init__(self,playerCount):
    '''PlayerSet(int) -> PlayerSet
    Constructs a group of players,
    given how many there are and asks the
    name of each player.'''
    self.playersList = []
    for i in range(self.playerCount):
        self.playersList.append(Player(input('Player ' + str(i+1) + ', please enter your name:')))
    self.currentPlayer = self.playersList[0]

which can become using list comprehension :

def __init__(self,playerCount):
    '''PlayerSet(int) -> PlayerSet
    Constructs a group of players,
    given how many there are and asks the
    name of each player.'''
    self.playersList = [Player(input('Player ' + str(i+1) + ', please enter your name:')) for i in range(self.playerCount)]
    self.currentPlayer = self.playersList[0]

Str

You probably should add a get_description method on players to handle the player.get_name() + ' has ' + str(player.get_score()) + ' points' part. Then, your __str__ method could/should use join : return "\n".join(p.get_description() for p in self.playersList).

go_to_next_player

The pythonic way to loop is to avoid using the length of the list.

for i in range(len(self.playersList)):
    something(self.playersList[i])

could/should be written :

for p in self.playersList:
    something(p)

Also, everything could be much simpler if self.currentPlayer was the index of a Player and not a Player object.

def go_to_next_player(self):
    '''PlayerSet.go_to_next_player()
    Moves the current player to the next
    player.'''
    self.currentPlayer = (self.currentPlayer + 1) % len(self.playersList)

(The constructor needs to be updated accordingly to set currentPlayer to 0).

is_curr_winner

This could simply be : return self.playersList[self.currentPlayer].get_score() >= 100.

Also, maybe you should add a is_winner method on the Player.

play_game

You shouldn't compare to True and False.

I am not sure if there's a point in returning a string.

I don't know enough about the logic of your game but is there a point in checking if the player has won before he plays if we do it after he plays anyway ?

Answer 3

Do not create players in __init__(). Pass a list of players into constructor instead of count. This will make your class more flexible. Also you class should not be bound to the UI implementation, so do not read data from a user in __init__().