3
\$\begingroup\$

Is there any room for improvement, in terms of simplicity and efficiency?

from random import randint

class Dice():

    def __init__(self):
        """Making a dice instance with 6 sides."""
        self.sides = 6

class Roller(Dice):

    def __init__(self,name,age):
        """Making a Dice Roller Instance"""
        super().__init__()
        #inhert sides attribute of a dice
        self.name = name
        self.age = str(age)

    def describe_roller(self):
        """Describe the information of the roller"""
        print('name: ' + self.name.title())
        print('age: ' + self.age)

    def roll_multiple(self,t):
        """Roll the dice multiple times and keep count in a dict"""
        count = {}
        one = 0
        two = 0
        three = 0
        four = 0
        five = 0
        six = 0
        for i in range(0,t):
            key = randint(1, self.sides)

            if key == 1:
                one += 1
                count["one"] = one
            elif key == 2:
                two += 1
                count["two"] = two
            elif key == 3:
                three += 1
                count["three"] = three
            elif key == 4:
                four += 1
                count["four"] = four
            elif key == 5:
                five += 1
                count["five"] = five
            elif key == 6:
                six += 1
                count["six"] = six
        print(count)


mike = Roller('mike',25)
mike.roll_multiple(6)
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$
  1. 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()
  2. 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.
  3. 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}')
  4. You don't need the empty parantheses in class Dice()
  5. You should put a space after the comma between arguments in a function call: mike = Roller('mike',25) becomes mike = Roller('mike', 25)
  6. 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.
  7. roll_multiple(self, t): t is not a particularly expressive variable name, something like times would be better for readability.
  8. Adding type hints (PEP 484) increases the readability of your code.
  9. 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.
  10. 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))
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Overall agree. Drop the __init__ -> None; refer stackoverflow.com/questions/64933298/… \$\endgroup\$
    – Reinderien
    Commented May 25, 2021 at 18:32
  • 1
    \$\begingroup\$ ...the above link actually argues in favour of an explicit None; github.com/python/mypy/issues/604 for more nuance \$\endgroup\$
    – Reinderien
    Commented May 25, 2021 at 18:34
  • 2
    \$\begingroup\$ Interesting, thanks for providing the discussion links. I've also found the return type annotation for __init__ rather unnecessary but I usually go full type hint for reviews (when recommending type hints at least). \$\endgroup\$ Commented May 25, 2021 at 20:08
  • 1
    \$\begingroup\$ Oh wow. This is the guidance and advice that I really needed. The result counting and storing part advice really hit the spot. Thank you for taking your time explaining and simplifying. I really really appreciate it. This really motivates me to code and learn more. Thank you so much. \$\endgroup\$ Commented May 26, 2021 at 3:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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