3
\$\begingroup\$

I've created a code based upon a dice roll where the number of rolls is decided by user input. I am new to this, so any and all feedback will be appreciated as I feel the code is quite clunky at best.

number_of_times = int(input("How many times do you want to roll the dice?: "))
print(number_of_times)

def dice_roll(x):
    import random
    for r in range(x):
        roll = random.randint(1,20)
        print(roll)

dice_roll(number_of_times)
\$\endgroup\$
3
\$\begingroup\$
  1. You should have imports at the top of your code. Having them in the middle of code makes it needlessly confusing with no benefit.
  2. You code doesn't account for the possibility of a user not entering an integer as input. You can utilize a while, try and except ValueError to allow a user to attempt to re-enter a value.
  3. You should keep code out of the global scope. Wrapping your code in a main function will help prevent future mishaps.
  4. You should really use a better name than x, amount is descriptive.
  5. Rather than r you can use _ or __ to denote that you are throwing away the value. This is no different that what you have done now, but it is a custom to help others understand your code quickly.
import random


def roll_dice(amount):
    for _ in range(amount):
        print(random.randint(1, 20))


def main():
    while True:
        try:
            amount = int(input("How many times do you want to roll the dice? "))
            break
        except ValueError:
            print("That is not a valid whole number.")
    print(amount)
    roll_dice(amount)


main()

There are some more advanced things you can do too.

  1. You can use an if __name__ == "__main__": guard to help prevent your main code from running accidentally. If it is imported by accident from another script.
  2. It would be better for you to return the values of the dice rather than print. This allows you to reuse the function later if you need to use the results of the thrown dice.
  3. You can use random.choices to select k amount of values.
  4. Print is slow. You may want to change to use "\n".join to reduce the overhead from calling print multiple times. However you need to convert all the values to strings.
  5. To easily convert each value in a list to a string you can use a for loop, building a new list.

    output = []
    for item in roll_dice(amount):
        output.append(str(item))
    

    This contains a lot of noise, so we can use a list comprehension to simplify this.

    output = [str(die) for die in roll_dice(amount)]
    
import random


def roll_dice(amount):
    return random.choices(range(1, 21), k=amount)


def main():
    while True:
        try:
            amount = int(input("How many times do you want to roll the dice? "))
            break
        except ValueError:
            print("That is not a valid whole number.")
    print(amount)
    print("\n".join([str(die) for die in roll_dice(amount)]))


if __name__ == "__main__":
    main()

Note: Personally I don't think using the list comprehension and str.join to be as nice as a for loop when it comes to non-performance critical code. This is to show you how you can improve future code with the code you have today.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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