0
\$\begingroup\$

I have this code written and I wanted to get a better performance out of it. Could somebody help me please? If possible I would like my code to get a better performance when rolling high numbers and when it getting to the percentage operation.

#implementation of multiple dice rolls at the same time and showing what each dice rolled individually
import random
from collections import Counter

#Welcoming the user to the game
print("Hello and welcome to the dice roller!")
name = input("What is your name?: ")
print ("Hi", name)

while True:
   #Asking the user if he wants to play to make it break the loop and say bye whenever he stops playing
   wants_to_play = input("Do you want to play? (y/n): ").lower()
   
   #if statement to determine the number of dice faces, and to "roll the dice" hypothetically.
   if wants_to_play == "y":
      dice_face = int(input("Select the number of faces you want your dice to have. WARNING: if you don't select a whole number, the dice roller won't work. "))
      dice_amount = int(input("Select how many dices of the given faces you want to roll: "))

      #generating dice roll number with the given input
      dice_rolls = []
      for i in range(0, dice_amount):
         dice_rolls.append(random.randint(1,dice_face))

      dice_rolls = []
      for i in range(0, dice_amount):
         dice_rolls.append(random.randint(1,dice_face))
      
      #Counting how many of each number are they and defining percentages
      num = Counter(dice_rolls)
      percentages = sum(num.values())
      total = round(percentages, 1)


      #printing final message output to the user
      print("Your dice percentages where: ")
      for face, count in num.most_common():
         print(f"{face}: {count} ({count/total:.2%})")

      
   #When the user stops wanting to play, saying them goodbye
   else:
      print("Well, it was great while it lasted! Until next time!")
      break


print("Developed by SMB Studios")
\$\endgroup\$
4
  • 2
    \$\begingroup\$ "Die" is singular, "Dice" is plural. Also, why do you do ` dice_rolls = [] for i in range(0, dice_amount): dice_rolls.append(random.randint(1,dice_face))` twice? \$\endgroup\$ Sep 28, 2022 at 14:16
  • 3
    \$\begingroup\$ Welcome to Code Review! To help reviewers give you better answers, we need to know what the code is intended to achieve. Please add sufficient context to your question to describe the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. Also, edit the title to simply summarise the task, rather than your concerns about the code. \$\endgroup\$ Sep 28, 2022 at 14:17
  • 1
    \$\begingroup\$ Also, your game exits when anything other than Y or y is entered, instead of just N or n. Presumably you want to ask the user to input their choice again? \$\endgroup\$ Sep 28, 2022 at 14:18
  • \$\begingroup\$ And this might help with getting the percentage values more elegantly: stackoverflow.com/a/24273934/4499183 \$\endgroup\$ Sep 28, 2022 at 14:21

1 Answer 1

1
\$\begingroup\$

First off, I would say your code doesn't do what it's stated to do:

#implementation of multiple dice rolls at the same time and showing what each dice rolled individually

It doesn't show the individual rolls, only the cumulative stats. If this is intentional, then fine, but I suggest you change the docstring (and make it one) to show that.

PEP-8

PEP-8 is a set of recommendations regarding the format of Python code. They mean that there is a standard way of writing Python code which makes it easier to read and use others' code.

This includes recommendations on indent levels (4 spaces), spaces after commas, comments, etc. I suggest you look into getting a linter such as flake8 or pylint and run your code through them to standardise it against others' codes.

Comments

As it is, many of your comments don't really help me understand the code any more than the code does and in some cases are somewhat misleading:

# if statement to determine the number of dice faces, and to "roll the dice" hypothetically.

The if statement doesn't determine the number of faces. It's whether the code will run or not. I'm not sure what you mean by

"roll the dice" hypothetically

Do you mean you're not really rolling dice because it's a computer?

Comments are very much personal choice, but I would usually just outline what a block of code is to do in simple terms, and in complex parts how it does so (algorithm name, references, etc).

Handling user input

Currently, your code crashes if you enter a non-int value into your input.

We can do better than this using a function to get an integer from a user:

def get_int(prompt: str) -> int:  # Type hints tell a user what to provide and expect
    """ Get a valid integer from the user """
    while True:
        val = input(prompt)
        try:
            val = int(val)
        except ValueError:  # If it can't be turned into an int, 
                            # it will raise this error, and this code will run 
            print(f"Invalid integer value ({val}), please try again.")
        else:  # If there isn't an error, this code will run   
            return val  # Set the name of the function where it is call 
                        # to this value and leave the function

Which means we now use:

dice_face = get_int("Select the number of faces you want your dice to have: ")
dice_amount = get_int("Select how many dice of the given faces you want to roll: ")

Duplicated effort

In your code, you roll the dice, discard them, then roll them again:

        dice_rolls = []
        for i in range(0, dice_amount):
            dice_rolls.append(random.randint(1,dice_face))

        dice_rolls = []
        for i in range(0, dice_amount):
            dice_rolls.append(random.randint(1,dice_face))

This means you spend a lot of effort doing nothing.

The other thing to bear in mind is that you are using append. List comprehensions (which you may not have met yet) are faster in general.

dice_rolls = [random.randint(1, dice_face) for _ in range(dice_amount)]  # 0 is implied in the range

The other thing is that you are building this list of values, and then only using the Counter to do consume the list. You might want to instead, just directly increment the counter.

num = Counter()
for i in range(dice_amount):
    roll = random.randint(1, dice_face)
    num[roll] += 1

or (advanced) you could use a generator expression as the iterable of the counter:

num = collections.Counter(random.randint(1, dice_face) for _ in range(dice_amount))

Here:

      percentages = sum(num.values())
      total = round(percentages, 1)

percentages doesn't compute the percentages, just the counts and is not really used again, and total being the rounding of this is irrelevant because the counts will always be integral. There is already a method on Counter (total) which does this.

            print(f"{face}: {count} ({count / num.total():.2%})")

Summary

Putting all this together, along with Tamoghna Chowdhury's comments, we end up with something like:

"""
Code to roll multiple dice and show individual rolls as well as aggregated statistics
"""
import random
from collections import Counter


def get_int(prompt: str) -> int:
    """ Get a valid integer from the user """
    while True:
        val = input(prompt)
        try:
            val = int(val)
        except ValueError:
            print(f"Invalid integer value ({val}), please try again.")
        else:
            return val


# Welcome user to the game
print("Hello and welcome to the dice roller!")
name = input("What is your name? ")
print("Hi", name)

while True:
    # Ask if user wants to play
    wants_to_play = input("Do you want to play? (y/n) ").lower()

    if wants_to_play == "y":
        # Get the number of faces, and number to roll.
        dice_face = get_int("Select the number of faces you want your dice to have: ")
        dice_amount = get_int("Select how many dices of the given faces you want to roll: ")

        # generate rolls
        num = Counter()
        for i in range(dice_amount):
            roll = random.randint(1, dice_face)
            print(f"Die {i}: {roll}")  # N.B. May want to add capability to disable this for large numbers of rolls (e.g > 10)
            num[roll] += 1

        # print stats
        print("\nYour dice percentages were: ")
        for face, count in num.most_common():
            print(f"{face}: {count} ({count / num.total():.2%})")

    elif wants_to_play == "n":
        # say goodbye and quit
        print("Well, it was great while it lasted! Until next time!")
        break

    else:
        print(f"Unknown option ({wants_to_play}), please try again.")

print("Developed by SMB Studios")
\$\endgroup\$
0

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.