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
\$\endgroup\$
  • 2
    \$\begingroup\$ This doesn't really have to do with the quality of the code, but "dice" is plural, "die" is singular. \$\endgroup\$ – Kevin Jul 31 '17 at 13:05
  • \$\begingroup\$ A semantic point: you're not really calculating probability, but frequency. \$\endgroup\$ – Octopus Jul 31 '17 at 18:57
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))
\$\endgroup\$
  • 1
    \$\begingroup\$ I wouldn't say that manually seeding a PRNG is that important. Python uses "good enough" defaults for the seed. \$\endgroup\$ – Phylogenesis Jul 31 '17 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 Jul 31 '17 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\$ – hjpotter92 Jul 31 '17 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.)

\$\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).

\$\endgroup\$
  • \$\begingroup\$ why 2 different replies? \$\endgroup\$ – hjpotter92 Jul 31 '17 at 16:27
  • \$\begingroup\$ So basically a double underline means nothing? \$\endgroup\$ – raven rogue Jul 31 '17 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 Aug 1 '17 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.

\$\endgroup\$
  • 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 Jul 31 '17 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\$ – opa Jul 31 '17 at 13:45

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.