Simple Python Dice [closed]

Question

I made a really simple Dice class and test script in Python as a gist. It is simpler than other dice I found on StackExchange being here and here. I made it simple for modularity and reusability. Granted, there are some quirks such as its max value being determined by a system's word size that may hinder its use. The word size is specific to Python 3.x. Should I make it usable for Python 2.7 and 3.x? I think calling it dice is a misnomer and should be fixed as well. I chose to have exceptions instead of assertions as they can be handled. I am throwing generic exceptions and specific descriptions which I think could be improved by having a more specific exceptions thrown. I think the testing script should probably be class as well. I followed the Sphinx commenting standard but probably did some boilerplate because I think it can be generated. I am interested in getting feedback from others.

dice.py

'''
.. module:: dice
.. moduleauthor:: Jack Bartolone <[email protected]>
'''

import random
import operator
import sys

class Dice(object):
    '''
    Python 3 dice used to roll a random an integer 
    between 3 to 2^63 -1 for 64-bit machines and 3 to 2^31-1 for 32-bit machines.
    '''
    #constants
    MINSIDES = 3 

    #constructors
    def __init__(self, sides):
        '''
        Intializes dice with :param sides

        :param sides: sides of the dice
        :raises: Exception for out of bounds and non-integer values
        '''
        self.sides = sides

    #range check
    sides = property(operator.attrgetter('_sides'))

    @sides.setter
    def sides(self, value):
        '''
        Throws exceptions for invalid input.

        :param value: setter value
        '''
        if (value <= self.MINSIDES): 
            raise Exception('Dice can must have at least three sides')
        elif (value > sys.maxsize): 
            raise Exception('Dice value exceeds max int size')
        elif (isinstance(value, int)):
            raise Exception('Dice value is not an integer')
        else:
            self._sides = value

    #methods
    def roll(self):
        '''
        Rolls dice. 

        :returns: pseudorandom integer between 1 and its sides 
        '''
        result = random.randint(1, self.sides)
        return result

dicetest.py

'''
.. module:: dice
.. moduleauthor:: Jack Bartolone <[email protected]>
'''

import pytest
import dice
import sys

def testlowerbounds():
    '''Test if numbers < 3 throw exception'''
    with pytest.raises(Exception):
        dice(1)

def testinbounds():
    '''Test if sys.maxsize + 1> numbers >= 3 not throw exception'''
    with pytest.raises(Exception):
        dice(3)

def testupperbounds():
    '''Test if sys.maxsize + 1 > numbers throw exception'''
    with pytest.raises(Exception):
        dice(sys.maxsize + 1)

def testisaninteger():
    '''Test if throwing exception for non-integer value'''
    with pytest.raises(Exception):
        dice('word')

testlowerbounds()
testinbounds()
testupperbounds()
testisaninteger()
#print docs
help(Dice)

Answer 1

Your limits are inconsistent: you can’t set the number of sides to the declared minimum: 3

if (value <= self.MINSIDES): 
        raise Exception('Dice can must have at least three sides')

(And the message: “... can must...”? It is not technically code, but ew!)

---

I've been trying to reconcile the above issue (can't set the number of sides to 3) with the the fact that you have a testinbounds() method which explicitly tests that exact case. Why weren't you able to catch that issue and fix it, before posting the code to Code Review.

I finally understand: the test code does not come close to working!

import dice

dice(3)                 # error: calling a module
help(Dice)              # error: undefined class

You need:

import dice

dice.Dice(3)            # construct a 3-sided `Dice` object from `dice` module
help(dice.Dice)         # print docs for `Dice` class in `dice` module.

or

from dice import Dice   # add class `Dice` to the current scope

Dice(3)                 # construct a 3-sided `Dice` object
help(Dice)              # print docs for `Dice` class

Answer 2

You spent 55 lines and a stateful class on something that can be accomplished in 3 lines (1 line if you don't count the imports) using only standard library functions:

from functools import partial
from random import randint

roll_six_sided_die = partial(randint, 1, 6)

if __name__ == '__main__':
    print(roll_six_sided_die())

Okay, we can make a factory method if you really want:

def make_die_roller(sides):
    if value < 2:
        raise ValueError('Must have at least 2 sides')
    return partial(randint, 1, sides)

This is vastly simpler for any developer to read and understand.

Design Philosophy - Classes aren't automatically better

Jack Diederich's Stop Writing Classes talk during PyCon US 2012 talks about this sort of class specifically. His advice is:

The signature of, "This shouldn't be a class," is that it has two methods, one of which is __init__. Anytime you see that, you should probably think, "Hey, maybe I just need the one method."

Or rephrased, if you have a class with an __init__ and one instance method, your class is functionally equivalent to just the method. In other words, it's bad to create a bunch of boilerplate structure just for the sake of having structure. Adding a class around a single method and then converting one parameter to an instance variable doesn't make the code better.

Python treats functions/methods as first class objects, meaning you can assign them to variables, pass them as parameters, and generally do any of the things you might want to do with an object. As a result, object oriented programing is de-emphasized in Python code, and procedural approaches are both widely accepted and often much simpler, especially if you minimize state (taking a sort of opportunistically functional mindset). Python makes this even easier since pinning parameters to a function (the partial call) is trivial.

Modularity and Reusability

Modularity is not a magic property that falls out automatically by stuffing everything into classes. It comes from doing the hard work of identifying the logically coherent pieces of your code. These are the elements that you can't (or at least don't) use separately, or they manage some single "thing." Yes, this is kind of vague, but that's partly because the term "modularity" is vague to begin with. It's also subjective and requires a great deal of intuition to do well.

Reusability is also not something that automatically falls out of putting functionality in classes. It comes from identifying elements of functionality that you need in many places, centralizing that logic's implementation, and ensuring it's parameterized with any inputs that vary across usage. Again, there's a lot of subjectivity and intuition here. It's not easy to just up and decide, "I need to make this piece reusable."

What to focus on instead

I've generally found that my primary focus should not be on modularity, reusability, or any vague terms like those. The question you want to ask yourself constantly is, "Am I making this thing easier or harder for some poor schmuck (like myself in a year when I've completely forgotten this code) to figure out later?" How do you make things easier to figure out later? You write your code so that it makes your thought process blatantly obvious. This means naming things so that reading them makes the meaning and intended usage jump out very quickly. It means leaving a few comments explaining why you did something when that can't be made obvious right in the code. It means abstracting away details that aren't important to the immediate flow of logic in a method.

With this mindset, you may start to notice little details, "Hey, this would be easier to understand if I stuff the logic for what I'm doing to each element of the list into a named function." Then suddenly, you find yourself needing that transformation all over the place. In other words, with some experience and practice, reusability and modularity start falling out semi-naturally when your primary focus is on making the code easier to understand. Habitually making small organizational changes to make your thought process jump out of the screen in your code can result in huge improvements in quality.

Other Notes

• Introducing new, modifiable state when it's not necessary is a huge no-no in any language. It makes your code more complicated and harder to understand and get right. Whatever implementation you use for this, it should not be possible to change the number of sides once the die is created. At a bare minimum, ditch the setter method for the number of sides.

• Use ValueError for invalid method arguments.

• Don't be overly zealous with your parameter testing. Python is dynamically typed; don't check for int using isinstance like that. If you want type checking that badly, look into type annotations and the checkers built up around them. There's no functional reason to require at least 3 sides. 2 will work just fine; this is effectively a coin.

• A single line condition in if statements does not need to be surrounded with parentheses. These should be omitted. Use if value < 2: instead of if (value < 2):.

• Avoid variables that don't exist for a reason. In your roll method, you assign to result and then immediately return it. This means you could have just returned it directly: return random.randint(1, self.sides).

Answer 3

A few remarks:

1. A two-sided dice is perfectly fine. It is called a coin.

2. Why limit the sides to sys.maxsize, when Python has arbitrary precision integers (not that you really need such a large dice)?

   import random
   import sys
   print(random.randint(sys.maxsize, sys.maxsize + 10))
   # 2147483655
   

3. The conditions of if, elif and else (and also for, while, with) do not need to be in parenthesis. They can be any expression, so just write

   if value < self.MINSIDES:
       ...
   

4. An improvement could be to make a dice class for RPGs, which is able to parse a dice string like "2d6+1", meaning roll two six-sided dice and add one. You can use e.g. the regex r'(\d+)d(\d+)\+?(\d*)', to parse that string.

Answer 4

Throwing an error when the sides argument is not an integer is, IMO, unpythonic. Credit is due though - it's not a hard type check.

Still, there's no logical reason why sides=3.0 would be any worse than sides=3. This design forces the end-user to conform to your preconceived input type to use the class.

I would suggest simply casting all inputs to ints instead. Any inputs that don't comply with the conversion will raise errors on their own - EAFP. If you want to be extra-dilligent, you could warn the user about the casting and/or catch and reraise unhandled conversion errors as your own error instances to make it more end-user-friendly.

---

On the modularity and extensibility side, you're explicitly supporting only dice with a linearly increasing number on each side.

This is a project scope decision, but a general solution would be to abstract the sides away from numbers, and instead have an indexed iterable of dice sides and randomly pick the index, which would point to an object storing the actual value on the side.

Answer 5

Wow, tests are here, that's nice.

• I guess I would add a newline on the end of the file as it is W292 in PEP-8. You can use a flake8 tool to check other style problems.

• About your question about python 2 and 3, you definetly should use only 3. Official support for python 2 will end in 2020 and no new features, just bugfixes. Of course it is nice to know python 2 syntax and problems, but development of new things should happen only in python 3.

• You are asserting if the Exception was raised in your code, but not the actual error message. You cannot be sure if you are in the correct point in your application. I would implement custom exceptions and try to assert them, as Exception would be anything that inherits from the Exception class, for example ZeroDivision Error.

Answer 6

Why not use magic methods and standard library? Duck typing is your friend and lets you use the standard library for all sorts of things.

import random

class Dice:
    def __init__(self, sides):
        self._len = sides

    def __len__(self):
        return self._len

    def __getitem__(self, x):
        return x + 1

d = Dice(6)
random.choice(d)
random.choices(d, k=10)