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
-
2\$\begingroup\$ This doesn't really have to do with the quality of the code, but "dice" is plural, "die" is singular. \$\endgroup\$Kevin– Kevin2017年07月31日 13:05:38 +00:00Commented Jul 31, 2017 at 13:05
-
\$\begingroup\$ A semantic point: you're not really calculating probability, but frequency. \$\endgroup\$Octopus– Octopus2017年07月31日 18:57:35 +00:00Commented Jul 31, 2017 at 18:57
5 Answers 5
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))
-
1\$\begingroup\$ I wouldn't say that manually seeding a PRNG is that important. Python uses "good enough" defaults for the seed. \$\endgroup\$Phylogenesis– Phylogenesis2017年07月31日 12:53:06 +00:00Commented 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\$raven rogue– raven rogue2017年07月31日 13:30:59 +00:00Commented Jul 31, 2017 at 13:30
-
\$\begingroup\$ @ravenrogue en.wikipedia.org/wiki/Snake_case and
ROLLED = ...
creates a dictionary with1..6
as keys, and initialised to a value0
. The loop then increments each key based on the values generated byrandom.randint(1, 6)
. \$\endgroup\$hjpotter92– hjpotter922017年07月31日 14:10:42 +00:00Commented Jul 31, 2017 at 14:10
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.)
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).
-
\$\begingroup\$ why 2 different replies? \$\endgroup\$hjpotter92– hjpotter922017年07月31日 16:27:26 +00:00Commented Jul 31, 2017 at 16:27
-
\$\begingroup\$ So basically a double underline means nothing? \$\endgroup\$raven rogue– raven rogue2017年07月31日 23:39:44 +00:00Commented 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\$MarianD– MarianD2017年08月01日 12:37:15 +00:00Commented Aug 1, 2017 at 12:37
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.
-
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\$Chris H– Chris H2017年07月31日 12:43:04 +00:00Commented 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\$Krupip– Krupip2017年07月31日 13:45:12 +00:00Commented Jul 31, 2017 at 13:45
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