1
\$\begingroup\$

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()
\$\endgroup\$
  • \$\begingroup\$ Is there a reason for calling a dice "die"? BTW you can find some Python style guides here \$\endgroup\$ – Rik Poggi Jul 23 '12 at 10:01
  • \$\begingroup\$ Take also a look here to know why you shouldn't do def __init__(self,rolls=[]): \$\endgroup\$ – Rik Poggi Jul 23 '12 at 10:07
  • \$\begingroup\$ 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. \$\endgroup\$ – jlim Jul 23 '12 at 15:46
2
\$\begingroup\$

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)]
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – jlim Jul 23 '12 at 15:52
  • \$\begingroup\$ @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. \$\endgroup\$ – Joel Cornett Jul 23 '12 at 17:14
  • \$\begingroup\$ @jlim: Updated my answer. This version will handle the possiblity of n ties, provided that n does not exceed the recursion limit ;) \$\endgroup\$ – Joel Cornett Jul 23 '12 at 19:36
  • \$\begingroup\$ Thank you for the update. Sorry I don't have enough rep to up vote yet. \$\endgroup\$ – jlim Jul 24 '12 at 1:33
  • \$\begingroup\$ @jlim: No problem man. Glad to help \$\endgroup\$ – Joel Cornett Jul 24 '12 at 2:02
3
\$\begingroup\$
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]
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – jlim Jul 24 '12 at 1:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.