Die-rolling for ordering

Question

I have a little experience with Python and am trying to become more familiar with it. As an exercise, I've tried coding a program where the ordering of a list of players is determined by the roll of a die. A higher number would place the player ahead of others with lower numbers. Ties would be broken by the tied players rolling again.

Here is an example:

 player 1, player 2, player 3
 rolls 6, rolls 6, rolls 1
 / \
 / \
 player 1, player 2 player 3
 rolls 5, rolls 4
 / \
 / \
 player 1 player 2

Final order: player 1, player 2, player 3

I'm interested in how I could have done this better. In particular, I'm not familiar with much of the Python style guide so making it more pythonic would be nice. Also, I'm not that familiar with unit testing. I've used unittest instead of nose since I was using an online IDE that supports it.

from random import randint
from itertools import groupby
import unittest
def rank_players(playerList, die): 
 tree = [playerList] 
 nextGeneration = []
 keepPlaying = True
 while keepPlaying:
 keepPlaying = False
 for node in tree: 
 if len(node) == 1:
 nextGeneration.append(node)
 else:
 keepPlaying = True
 rolls = [die.roll() for i in range(len(node))]
 turn = sorted(zip(rolls,node), reverse=True)
 print 'players roll:', turn
 for key, group in groupby(turn, lambda x: x[0]):
 nextGeneration.append(list(i[1] for i in group))
 tree = nextGeneration
 nextGeneration = []
 return [item for sublist in tree for item in sublist]
class Die:
 def __init__(self,rolls=[]):
 self.i = -1
 self.rolls = rolls
 def roll(self):
 self.i = self.i + 1
 return self.rolls[self.i]
class RankingTest(unittest.TestCase):
 def testEmpty(self):
 die = Die()
 players = []
 self.assertEquals(rank_players(players,die),[])
 def testOnePlayer(self):
 die = Die()
 player = ['player 1']
 self.assertEquals(rank_players(player,die),['player 1'])
 def testTwoPlayer(self):
 die = Die([6, 1])
 players = ['player 1', 'player 2']
 self.assertEquals(rank_players(players,die),['player 1', 'player 2'])
 def testThreePlayer(self):
 die = Die([6, 6, 1, 4, 5])
 players = ['player x', 'player y', 'player z']
 self.assertEquals(rank_players(players,die),['player x', 'player y','player z'])
 def testRunAll(self):
 self.testEmpty()
 self.testOnePlayer()
 self.testTwoPlayer()
 self.testThreePlayer()
if __name__ == '__main__':
 unittest.main()

• Rik Poggi: Is there a reason for calling a dice "die"? BTW you can find some Python style guides here
• Rik Poggi: Take also a look here to know why you shouldn't do def __init__(self,rolls=[]):
• jlim: Thanks for the links. I used the singular for dice, die, in an attempt to convey that each player would only do one roll for each turn. I would've liked a less ambiguous term.

Answer 1

Here's one way it could be done with a lot less code:

from itertools import groupby
from operator import itemgetter
from random import randint
def rank_players(playerList):
 playerRolls = [(player, randint(1, 6)) for player in playerList]
 playerGroups = groupby(
 sorted(playerRolls, key=itemgetter(1), reverse=True),
 itemgetter(1))
 for key, group in playerGroups:
 grouped_players = list(group)
 if len(grouped_players) > 1:
 for player in rank_players(zip(*grouped_players)[0]):
 yield player
 else:
 yield grouped_players[0]

Usage:

>>> print list(rank_players(["bob", "fred", "george"]))
[('fred', 5), ('george', 6), ('bob', 4)]

• jlim: I really appreciate your answer. I will look more carefully into it at the end of today. My impression is though, that this answer does not handle the case of two ties the way I was hoping to. For example, player 1 and player 2 tie with 5, player 3 is in the middle with 4, and player 4 and player 5 tie with 3. Thank you for your effort. I'm not sure of the procedure, but I will probably mark it as the answer by the end of today if it is the best available answer.
• Joel Cornett: @jlim: Ah, I see (hence your use of groupby.) Well this solution is easily extensible to that end, since the recursion works on any set of sublists of players. I am a little preoccupied at the moment, but I can probably post a modified solution in a couple of hours.
• Joel Cornett: @jlim: Updated my answer. This version will handle the possiblity of n ties, provided that n does not exceed the recursion limit ;)
• jlim: Thank you for the update. Sorry I don't have enough rep to up vote yet.
• Joel Cornett: @jlim: No problem man. Glad to help

Answer 2

from random import randint
from itertools import groupby
import unittest
def rank_players(playerList, die): 

Python convention is to use names like player_list for arguments/local variables

 tree = [playerList] 

This isn't really used to hold a tree. Its only ever two levels deep, so its a list of lists. You also shouldn't name variables after datastructures, you should name after the meaning in the code.

 nextGeneration = []

You should move this into the loop. That way you won't have to repeat it later

 keepPlaying = True
 while keepPlaying:
 keepPlaying = False

Its best to try and avoid boolean logic flags. They tend to make code harder to follow. If you can, try to structure code to avoid their use.

 for node in tree: 
 if len(node) == 1:
 nextGeneration.append(node)
 else:
 keepPlaying = True
 rolls = [die.roll() for i in range(len(node))]

I'd have done:

 rolls = [ (die.role, item) for item in node]

This avoids the pointless call to len/range, and also handles the zipping in the next line

 turn = sorted(zip(rolls,node), reverse=True)
 print 'players roll:', turn
 for key, group in groupby(turn, lambda x: x[0]):
 nextGeneration.append(list(i[1] for i in group))

Joel's recursive solution is much nicer, I think.

 tree = nextGeneration
 nextGeneration = []
 return [item for sublist in tree for item in sublist]
class Die:
 def __init__(self,rolls=[]):

In general, don't assign mutable objects as defaults. Here I don't even see why you would have a default since the object is useless with the default constructor.

 self.i = -1

I'd use index, just to be more explicit

 self.rolls = rolls
 def roll(self):
 self.i = self.i + 1
 return self.rolls[self.i]

But here's how I'd write your function

def rank_players(player_list)
 random.shuffle(player_list)

All you are doing in the end is shuffling the list of players, and python has a function for that. Unless you really want to simulate dice, there isn't a lot of point in doing it.

Another approach, I'm not sure a good one, would be:

class Die(object):
 def __init__(self, player):
 self.player = player
 self.rolls = []
 def __getitem__(self, index):
 # if you don't provide an __iter__
 # python will use __getitem__
 # whenever an attempt is made to access
 # past the end, we just roll a new number
 if index == len(self.rolls):
 roll = random.randrange(6)
 print "Player: %s Rolls a: %d" % (self.player, roll)
 self.rolls.append(roll)
 return roll
 else:
 return self.rolls[index]
 def __cmp__(self, other):
 # the only Die I am equal to is myself
 if self is other:
 return 0
 # look at the rolls in both die to 
 # find first one that's different
 for a, b in itertools.izip(self, other):
 if a != b:
 return cmp(a,b)
 assert False
def rank_players(player_list):
 decorated = [(Die(player), player) for player in player_list]
 decorated.sort(reverse = True)
 print [die.rolls for die, player in decorated]
 return [player for die, player in decorated]

• jlim: Thank you for your input. I agree shuffle would be better, but I wanted to try out the dice rolling and node expansion idea. Sorry, I don't have enough rep to upvote yet. I will look over the rest of your answer later.