Roller
should not inherit from Dice
, as Roller
represents a human. Humans being a sub-class of Dice
is pretty unintuitive. Rather, Roller
should have an attribute: self.dice = Dice()
- There's no particular reason
Roller.age
should be of type str
. I'd keep it as an int
and only convert to str
if you need its string-represantation.
- f-Strings are a really convenient tool for creating strings containing variables / expressions. They also handle converting to
str
for you. Example: print(f'age: {self.age}')
- You don't need the empty parantheses in
class Dice()
- You should put a space after the comma between arguments in a function call:
mike = Roller('mike',25)
becomes mike = Roller('mike', 25)
roll_multiple
should be a method of Dice
that can be called by the Roller
. This is part of an intuitive API. Users (including yourself) will probably expect to roll the dice, not the human. If you need Roller.roll_multiple
, I'd only make it a wrapper for Dice.roll_multiple
.
roll_multiple(self, t)
: t
is not a particularly expressive variable name, something like times
would be better for readability.
- Adding type hints (PEP 484) increases the readability of your code.
- It's a convention in Python to name variables, whose values you don't actually use,
_
. So for i in range(0,t):
becomes for _ in range(t):
. Notice that you don't need to provide zero as the first argument as it's the default start for range
.
- Passing
sides
as a default argument to the constructor of Dice
makes the class a whole lot more versatile.
Algorithm
Hardcoding values like
one = 0
two = 0
three = 0
...
should always raise a red flag when writing code. This is neither necessary nor recommended in most situations. You already laid the groundwork by putting Dice.sides
in an attribute, now we need to use it. You also don't need to keep a seperate count of the occured values outside of the dict
. I implemented it to exactly mirror your functionality (i.e. using string-representations as keys for the dict and only counting numbers that actually occured), inside of class Dice
:
from random import randint
from collections import defaultdict
from num2words import num2words
...
def roll(self) -> int:
return randint(1, self.sides)
def roll_multiple(self, times: int) -> dict:
count = defaultdict(int)
for _ in range(times):
number = self.roll()
key = num2words(number=number)
count[key] += 1
return dict(count)
num2words
is a third-party library, that needs to be installed first.
If having keys of type int
is fine for your use case, I would recommend using them as they're way more usable for further calculations. You also don't necessarily need to cast to dict
, as a defaultdict
provides all the same basic functionality. It does make a difference when printing though. Here is the adjusted implementation:
def roll_multiple(self, times: int) -> defaultdict:
count = defaultdict(int)
for _ in range(times):
key = self.roll()
count[key] += 1
return count
Suggested code
from random import randint
from collections import defaultdict
class Dice:
def __init__(self, sides: int = 6):
"""Making a dice instance with a variable number of sides."""
self.sides = sides
def roll(self) -> int:
"""Returns the result of a single dice roll."""
return randint(1, self.sides)
def roll_multiple(self, times: int) -> defaultdict:
"""Provides the results of a variable number of dice rolls in a defaultdict."""
count = defaultdict(int)
for _ in range(times):
key = self.roll()
count[key] += 1
return count
class Roller:
def __init__(self, name: str, age: int) -> None:
"""Making a Dice Roller Instance"""
self.name = name
self.age = age
self.dice = Dice()
def describe_roller(self) -> None:
"""Describe the information of the roller"""
print(f'name: {self.name.title()}')
print(f'age: {self.age}')
def roll_multiple(self, times: int) -> None:
"""Roll the dice multiple times and print the result"""
print(self.dice.roll_multiple(times))