Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I know there is a better way to store data. What is the most concise way to simplify this script?

from random import randint
from sys import exit
import os

os.system('clear')
print "Welcome to the dice rolling simulator!"
raw_input("Press enter to begin.")

total = 0

completed = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

def roll():
    os.system('clear')
    die1 = randint(1, 6)
    die2 = randint(1, 6)
    global total
    total = die1 + die2
    storetotal()
    print "Die 1: %d \nDie 2: %d \nTotal: %d." % (die1, die2, total)
    print "\n\nRoll again?"
    roll_again = raw_input("Press enter to roll again, type 'stats' to view scores, or 'quit' to exit.\n> ")    
    if roll_again == "":
        roll()
    elif roll_again == "stats":
        stats()
    elif roll_again == "quit":
        exit(0)
    else:
        print "I don't know what that means so you get to roll again."
        raw_input("> ")
        roll()

def stats():
    global total
    print "2s: %d \n3s: %d \n4s: %d \n5s: %d \n6s: %d \n7s: %d \n8s: %d" % (completed[0],
                                                    completed[1], completed[2], completed[3], 
                                                    completed[4], completed[5], completed[6])
    print "9s: %d \n10s: %d \n11s: %d \n12s: %d""" % (completed[7], completed[8], 
                                                    completed[9], completed[10]) 
    raw_input("")
    roll()

def storetotal():
    if total == 2:
        completed[0] += 1
    elif total == 3:
        completed[1] += 1
    elif total == 4:
        completed[2] += 1
    elif total == 5:
        completed[3] += 1
    elif total == 6:
        completed[4] += 1
    elif total == 7:
        completed[5] += 1
    elif total == 8:
        completed[6] += 1
    elif total == 9:
        completed[7] += 1
    elif total == 10:
        completed[8] += 1
    elif total == 11:
        completed[9] += 1
    else:
        completed[10] += 1


roll()
share|improve this question

2 Answers 2

up vote 12 down vote accepted

Line by line analysis

Use four spaces to indent python code. Never use tabs.

from random import randint
from sys import exit
import os

It is a good practice to do top-level imports (without importing module members). This way it's easier to understand what package the methods come from.

os.system('clear')
print "Welcome to the dice rolling simulator!"
raw_input("Press enter to begin.")

total = 0

completed = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

Use list comprehensions.

def roll():

Roll method is confusing. You expect it to roll a die, but it processes iterations of the game. A while loop would be more explicit here.

os.system('clear')
die1 = randint(1, 6)
die2 = randint(1, 6)
global total

Global variables are no-no. Never.

total = die1 + die2
storetotal()
print "Die 1: %d \nDie 2: %d \nTotal: %d." % (die1, die2, total)
print "\n\nRoll again?"
roll_again = raw_input("Press enter to roll again, type 'stats' to view scores, or 'quit' to exit.\n> ")    

You're exceeding recommended 80 column length limit.

if roll_again == "":
    roll()

See how I changed this block in a code below.

elif roll_again == "stats":
    stats()
elif roll_again == "quit":
    exit(0)

Return from the method is sufficient here.

else:
    print "I don't know what that means so you get to roll again."
    raw_input("> ")
    roll()

def stats():
global total
print "2s: %d \n3s: %d \n4s: %d \n5s: %d \n6s: %d \n7s: %d \n8s: %d" % (completed[0],                                                                     completed[1], completed[2], completed[3], 
                                            completed[4], completed[5], completed[6])
print "9s: %d \n10s: %d \n11s: %d \n12s: %d""" % (completed[7], completed[8], 
                                                completed[9], completed[10]) 

Indentation here is completely messed up.

raw_input("")
roll()

def storetotal():

PEP8: store_total.

if total == 2:
    completed[0] += 1
elif total == 3:
    completed[1] += 1
elif total == 4:
    completed[2] += 1
elif total == 5:
    completed[3] += 1
elif total == 6:
    completed[4] += 1
elif total == 7:
    completed[5] += 1
elif total == 8:
    completed[6] += 1
elif total == 9:
    completed[7] += 1
elif total == 10:
    completed[8] += 1
elif total == 11:
    completed[9] += 1
else:
    completed[10] += 1


roll()

Improved code

I simplified your roll method to roll2d6 and moved all the other code into main.

import random
import os


def roll2d6():
    die1 = random.randint(1, 6)
    die2 = random.randint(1, 6)
    return die1 + die2


def display_stats(stats):
    for i, result in enumerate(stats):
        print "%ds: %d" % (i + 2, result)


def update_stats(total, stats):
    stats[total - 2] += 1
    return stats


def main():
    stats = [0 for _ in xrange(10)]

    os.system('clear')
    print "Welcome to the dice rolling simulator!"
    raw_input("Press enter to begin.")
    while True:
        os.system('clear')
        total = roll2d6()
        stats = update_stats(total, stats)
        print "Die 1: %d \nDie 2: %d \nTotal: %d." % (die1, die2, total)
        print "\n\nRoll again?"
        roll_again = raw_input((
                "Press enter to roll again, type 'stats' to view scores, "
                "or 'quit' to exit.\n> "))
        if roll_again.lower() == "stats":
            display_stats(stats)
        elif roll_again.lower() == "quit":
            break
        elif roll_again.lower() != "":
            print "I don't know what that means so you get to roll again."

if __name__ == "__main__":
    main()
share|improve this answer
1  
Additional suggestion : stats = [0] * 10 –  Josay Apr 4 at 23:14
1  
Also : call lower() on the result of raw_input() before storing in roll_again / write elif roll_again.lower() == "": continue to have all conditions written in a positive way - it makes the code longer but also clearer imho. –  Josay Apr 4 at 23:22
    
Check highest voted answer: stackoverflow.com/questions/119562/… ... –  David Mulder Apr 5 at 1:25
    
display_stats(stats) would be cleaner with enumerate(stats, start=2) rather than i + 2 on every iteration and update_stats(total, stats) is just a waste of space, it is only called once and is basically a one-liner so why not inline it? –  codesparkle Apr 5 at 2:13
1  
There's a coding style that any logical selfcontained subunit should be def'd so it can be reused elsewhere. Space is cheap, and you won't notice any speed difference. –  Snowbody Apr 5 at 2:54

So, first, it's all a bit confusing when the indentation is messed up (which is important in Python). You should first get that in order.

Also globals are usually a bad idea, and they're not necessary here. It also appears that, even properly executed, you've built this program as an infinite recursion (which is very inefficient). Instead you should just have your code in a loop implemented as a finite state machine (one state for rolling, another for stats, and an exit-state for quitting.

Also python uses underscore_naming so storetotal should be store_total (or better yet update_total). Also storetotal can be simplified greatly to a single line:

completed[total - 2] += 1

And the stats printing lines can be similarly simplified:

for i in xrange(0, 11):
    print '%ds: %d' % (i + 2, completed[i])

From a style perspective, Python also uses single-quotes, not double-quotes.

The resultant code should look something like this:

from random import randint
from sys import exit
import os


def handle_roll(total_counts):
    die1, die2 = randint(1, 6), randint(1, 6)
    total = die1 + die2
    total_counts[total - 2] += 1

    print 'Die 1: %d \nDie 2: %d \nTotal: %d.' % (die1, die2, total)
    print '\n\nRoll again?'
    response = raw_input('Press enter to roll again, type "stats" to view scores, or "quit" to exit.\n> ').lower()
    if response == '':
        return handle_roll
    elif response == 'stats':
        return handle_stats
    elif response == 'quit':
        return None

    return handle_unknown


def handle_stats(total_counts):
    for i in xrange(0, 11):
        print '%ds: %d' % (i + 2, total_counts[i])
    raw_input('')
    return handle_roll


def handle_unknown(total_counts):
    print 'I don\'t know what that means so you get to roll again.'
    raw_input('')
    return handle_roll


def main():
    os.system('clear')
    print 'Welcome to the dice rolling simulator!'
    raw_input('Press enter to begin.')

    total_counts = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
    state = handle_roll
    while state != None:
        os.system('clear')
        state = state(total_counts)


if __name__ == "__main__":
    main()
share|improve this answer
1  
Using single or double quotes in python is a matter of preference. Also, PEP 257 enforces using double quotes for docstrings. Agree with the rest. –  Ruslan Osipov Apr 4 at 23:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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