Python dice class

Question

I've built a simple dice class utility for rolling dice. It supports weighted rolling, and usage is fairly simple. Here's the code.

from random import choice
class Die(object):
 """
 The Die class represents a sided die
 with n amount of sides. Sides can be
 weighted. For example:
 die = Die({
 "side_name": weight
 ...
 })
 """
 def __init__(self, dice_sides: dict):
 self.dice_sides = dice_sides
 self._choosing_data = []
 def _populate_choosing_data(self):
 """
 Populate self._choosing_data with data
 to choose from, based on weight.
 """
 for key, value in self.dice_sides.items():
 if type(value) == int:
 for weight in range(value):
 self._choosing_data.append(key)
 else:
 raise TypeError("Weight value needs to be an integer.")
 def select_side(self):
 """
 Select a random side from the list of
 sides based on their weights. The side 
 name will be returned as a string.
 """
 self._populate_choosing_data()
 return choice(self._choosing_data)

Here's some example usage. The dictionary structure is as follows. {"side name": weight}.

import dice_utils
die = dice_utils.Die({
 "1": 1,
 "2": 5,
 "3": 1,
 "4": 2,
 "5": 7,
 "6": 1
})
print(die.select_side())

I'd like some suggestions for improvement. I especially don't like how I have to allocate a private variable as well, so if there are any suggestions for improvement there, I'd like some.

• jonrsharpe: Why do you call _populate_choosing_data in select_side rather than __init__? If this is a worry about mutability, should a Die be mutable?

Answer 1

There is no need to explicitly inherit from object in Python 3 - all classes are new-style. And you can't do it for backwards compatibility if you're also using function annotations, which 2.x doesn't support.

---

Your docstrings aren't accurate, for example:

class Die(object):
 """
 The Die class represents a sided die
 with n amount of sides. ...

but there is no n in the code! Also, note that per the guidelines:

Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description.

---

.select_side() seems like an odd name for the method - typically, this would be named .roll(), as that's what you do with a Die!

---

You have some minor violations of the style guide, for example:

Avoid extraneous whitespace in the following situations:

• ...

• More than one space around an assignment (or other) operator to align it with another.

and yet:

self.dice_sides = dice_sides
self._choosing_data = []

---

if type(value) == int:

is not the correct way to test whether value is an integer, for two reasons:

1. int, like the other built-in types (and None), is a singleton, so you can and should use is int (identity) for comparison rather than == int (equality); and
2. isinstance(value, int) is a much better approach, as it supports inheritance correctly (e.g. if you create some int subclass, for example a PositiveNonZeroInt for the weights and number of sides, it will still be accepted).

---

Initialising _choosing_data in __init__ but populating it in _populate_choosing_data is a little awkward, and leaves you open to bugs... which you've introduced:

>>> die = Die({1: 1, 2: 2})
>>> die.select_side()
1
>>> die._choosing_data
[1, 2, 2]
>>> die.select_side()
1
>>> die._choosing_data
[1, 2, 2, 1, 2, 2] # oh dear!

This is a bit inefficient in the best case, where the dice_sides never get changed (the proportions of the different items remain the same) and can lead to incorrectly-distributed outputs in the worst case, where the user tries to alter the proportions.

Minimally, you can move self._choosing_data = [] to the start of _populate_choosing_data. However, it would probably be better to call _populate_choosing_data from __init__, and protect dice_sides as read-only using a property:

class Die(object):
 def __init__(self, dice_sides: dict):
 self._dice_sides = dice_sides.copy()
 self._populate_choosing_data()
 @property
 def dice_sides(self):
 return self._dice_sides.copy()
 ...

As dice_sides is a mutable object, note that I've introduced .copy() to prevent the user from accidentally changing the version that the instance uses internally.

---

There is also space for some inheritance here; consider:

class Die(object):
 def __init__(self, sides):
 self.sides = sides
 def roll(self):
 return random.randrange(self.sides) + 1
class WeightedDie(Die):
 def __init__(self, sides, weights):
 super().__init__(sides)
 self._weights = {side: weights.get(side, 1) 
 for side in range(1, sides+1)}
 ...
 def roll(self):
 ...

This lets the user specify the minimal information required; your example:

die = dice_utils.Die({1: 1, 2: 5, 3: 1, 4: 2, 5: 7, 6: 1})

becomes:

die = dice_utils.WeightedDie(6, {2: 5, 4: 2, 5: 7})

This only supports numerical sides at present (note that I've altered your example accordingly, and removed the extraneous whitespace), but could be adapted to allow non-numerical sides.

---

Weighted random choices are a pretty common problem, and have been solved in various ways that you could adopt; see e.g. "A weighted version of random.choice".

Answer 2

_populate_choosing_data does two things:

1. Validates member attribute
2. Populates "choosing data"

It would be better if it did just the one thing its name says.

The parameter name of the constructor is "dice_sides", but we're in a Die class, so dice is already implied clearly enough. What's not so clear is that the dictionary is a mapping of sides to weights. You could make it obvious by using a better name.

And when you iterate over this dictionary, key and value are generic names that don't mean much. It would be better to call the loop variables side and weight, to make this clear and intuitive.

Lastly, " choosing data " isn't a great name. How about "choices"?