"Pass the Pigs" game

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

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

-
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

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

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

-
Instead of:

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

consider:

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

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

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

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

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

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

StackExchange Code Review Q#32358, answer score: 6