7
\$\begingroup\$

I'm a beginner at Python, and to start off my learning progress I created a simple program which will roll a dice and decides the probability of it. It would be great to have criticism on my code, like how I can improve it, make it shorter, etc.

import random
rolled = []
rolledtimes = 0;
biggest = []
freq = int(input('How many times would you like to roll the dice? '))
def roll():
 rand = random.randrange(1,7)
 return rand
def probability():
 for i in range(0,6):
 print('Calculation of probability:')
 percentage = "{:.2f}".format((count[i] / freq)*100)
 percent = str(percentage) + '%'
 print(' ', i + 1, ':', percent)
def theoretical():
 result = "{:.2f}".format((1/6)*freq)
 denominator = "{:.2f}".format(((1/6)*freq)*6)
 print('\nIn theory, each dice should roll {} out of {} times'.format(result,denominator))
def findBiggest():
 for i in range(1,7):
 biggest.append(rolled.count(i))
 print('\n', 'The most times a dice is rolled is', max(biggest), 'times')
def findSmallest():
 for i in range(1,7):
 biggest.append(rolled.count(i))
 print('\n', 'The least times a dice is rolled is', min(biggest), 'times')
for i in range(1,freq + 1):
 number = roll()
 rolled.append(number)
 rolledtimes+=1
count = [rolled.count(1),rolled.count(2),rolled.count(3),rolled.count(4),rolled.count(5),rolled.count(6)]
print('After being rolled {} times:\n\n1 is rolled {} times\n2 is rolled {} times\n3 is rolled {} times\n4 is rolled {} times\n5 is rolled {} times\n6 is rolled {} times\n'.format(rolledtimes,count[0],count[1],count[2],count[3],count[4],count[5]))
probability()
findBiggest()
findSmallest()
theoretical()

When used, it will print out something like this in the console:

How many times would you like to roll the dice? 1000
After being rolled 1000 times:
1 is rolled 180 times
2 is rolled 161 times
3 is rolled 190 times
4 is rolled 145 times
5 is rolled 162 times
6 is rolled 162 times
Calculation of probability:
 1 : 18.00%
Calculation of probability:
 2 : 16.10%
Calculation of probability:
 3 : 19.00%
Calculation of probability:
 4 : 14.50%
Calculation of probability:
 5 : 16.20%
Calculation of probability:
 6 : 16.20%
 The most times a dice is rolled is 190 times
 The least times a dice is rolled is 145 times
In theory, each dice should roll 166.67 out of 1000.00 times
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Jul 31, 2017 at 8:57
\$\endgroup\$
2
  • 2
    \$\begingroup\$ This doesn't really have to do with the quality of the code, but "dice" is plural, "die" is singular. \$\endgroup\$ Commented Jul 31, 2017 at 13:05
  • \$\begingroup\$ A semantic point: you're not really calculating probability, but frequency. \$\endgroup\$ Commented Jul 31, 2017 at 18:57

5 Answers 5

3
\$\begingroup\$

For styling, variable naming etc. conventions, please take a look at PEP-8. Your globals should be UPPERCASED and function names should be snake_cased.

Since you are not at all interested in the rolled list, why store a thousand values in memory?

You should always seed when using a RNG function. For more information on why this practice is favoured, check this SO response.

Instead of using random.randrange, prefer using random.randint.

With those in place, you have:

import random
import operator
random.seed()
ROLLED = {i: 0 for i in range(1, 7)}
ITERATIONS = int(input('How many times would you like to roll the dice? '))
def probability():
 print("Calculation of probability: ")
 for key, count in ROLLED.items():
 print("\t{}: {:.2f}".format(key, count*100./ITERATIONS*1.))
for _ in range(ITERATIONS):
 ROLLED[random.randint(1, 6)] += 1
probability()

To find the most rolled, and least rolled face of the die, you can use a custom operator on ROLLED dictionary:

max(ROLLED.iteritems(), key=operator.itemgetter(1))
min(ROLLED.iteritems(), key=operator.itemgetter(1))
answered Jul 31, 2017 at 12:06
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I wouldn't say that manually seeding a PRNG is that important. Python uses "good enough" defaults for the seed. \$\endgroup\$ Commented Jul 31, 2017 at 12:53
  • \$\begingroup\$ Thank you for the comment. I am still confused, though, on what you mean by snake_cased. It would be great if you could clarify that. Additionally, I do not fully understand what this line of code does= ROLLED = {i: 0 for i in range(1, 7)} \$\endgroup\$ Commented Jul 31, 2017 at 13:30
  • \$\begingroup\$ @ravenrogue en.wikipedia.org/wiki/Snake_case and ROLLED = ... creates a dictionary with 1..6 as keys, and initialised to a value 0. The loop then increments each key based on the values generated by random.randint(1, 6). \$\endgroup\$ Commented Jul 31, 2017 at 14:10
2
\$\begingroup\$

Only some not very important things:

In

def probability():
 for i in range(0,6):

you used different range as in other function definitions - and without good reason.


The parameter (1/6)*freq)*6 of the format() method in the statement

 denominator = "{:.2f}".format(((1/6)*freq)*6)

(in your theoretical() function) is simly freq, so consider to substitute it with

 denominator = "{:.2f}".format(freq)

The (long) statement

count = [rolled.count(1),rolled.count(2),rolled.count(3),rolled.count(4),rolled.count(5),rolled.count(6)]

repeats 6 times the same pattern, so consider to substitute it with

count = [rolled.count(i) for i in range(1, 7)]

(This is so called list comprehension and the principle is called DRY - Don't Repeat Yourself.)

answered Jul 31, 2017 at 15:40
\$\endgroup\$
1
\$\begingroup\$

You don't need count number of appends (in the variable rolledtimes) in

for i in range(1,freq + 1):
 number = roll()
 rolled.append(number)
 rolledtimes+=1

as it is the same as the length(rolled) and in turn it is simply freq, so this loop may be substitued with

rolled = [roll() for __ in range(freq)]

(and the previous initialization of rolled may be indeed omitted.)

Explanation:

We wanted a list of freq times rolling, so we may use a list comprehension for it.
Instead of using a variable (e. g. for i in range(freq)) we use double underline (__) as we don't need save this value.

Note:

Double underline is preferred way (in comparison with the single underline symbol).

answered Jul 31, 2017 at 16:04
\$\endgroup\$
3
  • \$\begingroup\$ why 2 different replies? \$\endgroup\$ Commented Jul 31, 2017 at 16:27
  • \$\begingroup\$ So basically a double underline means nothing? \$\endgroup\$ Commented Jul 31, 2017 at 23:39
  • \$\begingroup\$ @ravenrogue - Double underline is something as a variable without access to it. It is a styling method for emphasizing: "The syntax required a variable here but in this particular case I don't need it as I will never use it." \$\endgroup\$ Commented Aug 1, 2017 at 12:37
1
\$\begingroup\$

The code it's self seems to work alright. But you should really be commenting your code. By commenting i mean using # to prefix a line or near the end of a line so that you can input text and for the interpreter to ignore it. This let's you explain what each part of your code is doing at vastly improves your readability. Other than that i'd say your fine but you should really be commenting your code.

Edit: As pointed out by Chris H, you shouldn't comment every part of the code, and only parts that require explanation. For instance if you had a lengthy piece of code that can be hard to interpret without comments. And yes you may not need to comment this particular piece of code but commenting on the less obvious parts are still good practice.

answered Jul 31, 2017 at 10:58
\$\endgroup\$
2
  • 6
    \$\begingroup\$ I thinkt his would be a better answer if it included something about how comments are for explaining code that requires explanation. Otherwise you end up with the kind of programmer who writes comments like def activateWidget(): #activates the widget. \$\endgroup\$ Commented Jul 31, 2017 at 12:43
  • \$\begingroup\$ Commenting this code shouldn't be necessary, The names of the functions and variables can be entirely self documenting in this case, the problem is that they aren't and they are ambiguous. \$\endgroup\$ Commented Jul 31, 2017 at 13:45
1
\$\begingroup\$

If it's okay to use a python library function, multinomial works

from scipy.stats import multinomial
dice = multinomial(1, [0.1, 0.3, 0.05, 0.2, 0.15, 0.2])
roll_dice = dice.rvs()

Check the output

roll_dice
array([[0, 0, 0, 0, 1, 0]])

And argmax returns the index of values 1

roll_dice.argmax()
4
answered Apr 13, 2020 at 4:00
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.