Interactive dice simulator

Question

After finishing my course on CodeAcademy, I wanted to write a program. Please provide opinions.

from random import randint
import sys, time, os
def clear(): #Usefull for clearing the terminal, should be cross-platform
 os.system('cls' if os.name=='nt' else 'clear')
def initialize_variables(): #Self Described
 global count, start, count_pos, initi_pos
 count = 0 
 start = ""
 count_pos = 2
 initi_pos = 0
def open_file(): #Opens the options' file
 global options_file
 options_file = open("options.txt", "a+")
def close_file(): #Closes the option's file
 global options_file
 options_file.close()
 #return options_file.closed #Debugging purposes
def initial_setup(): #
 global options_file
 global count_pos, initi_pos 
 options_file.seek(initi_pos, 0)
 if options_file.read(1) == "": #Check the first value of the file, to determine if the user is running the Dice Simulator (DS from now on)
 options_file.write("0") #If no value found, set it at default "0"
 options_file.seek(initi_pos, 0) #Reposition cursor
 if (options_file.read(1) == "0") : #If default value found, the user's running the program for the first time.
 print "Welcome to the dice simulator"
 print "It seems like you are here for the first time."
 options_file.seek(count_pos, 0) #Reposition cursor
 options_file.write(str(raw_input("After how many times you want to change the dice? "))+ "\n")
 print "Done!",
 for i in range(1,4):
 time.sleep(0.5)
 print ".",
 options_file.seek(initi_pos, 0) #Reposition cursor to initi value
 options_file.write("1") #Change it to our new default value
 time.sleep(2)
 clear() #Clear the window
 else:
 pass
def change_options():
 global options_file
 count_2file = int(raw_input("After how many rolls will you change the dice? "))
 options_file.seek(count_pos,0)
 options_file.write(str(count_2file))
 clear()
def menu():
 print "Dice Rolling Simulator |"
 print "-----------------------/"
 print "1. Start rolling"
 print "2. Change options"
 print "3. Quit"
 choice = int(raw_input("Your selection is: "))
 if choice == 1: #If first choice is selected
 game() #Start the game
 elif choice == 2: #If second choice is selected
 change_options() #Change the options
 menu() #Return to menu
 elif choice == 3: #If third choice is selected
 print "Application will now exit."
 time.sleep(3)
 sys.exit() #Kill the app.
 else: #INVALID INPUT
 print "Invalid input. Error code: 0x001" #Print the appropriate message
 print "Application will now exit."
 time.sleep(3)
 sys.exit() #Kill the app
def game():
 global start
 x = int(raw_input("The size of the dice(s) is: "))
 y = int(raw_input("How many times will you roll it? "))
 start = raw_input("Roll the dice (y/n)? ")
 while True:
 if start == "y":
 clear() 
 print roll_dice(x,y)
 start = roll_again()
 global count
 count += 1 
 promtp_change(count)
 elif start == "n":
 clear() #Clear screan
 print "Application will now exit."
 time.sleep(2)
 close_file() #Before exit make sure to close the file
 sys.exit()
 elif start != "y" and start != "n":
 clear()
 print "Invalid input. Error code: 0x001"
 print "Application will now exit."
 time.sleep(2)
 close_file() #Before exit make sure to close the file
 sys.exit()
 ###-###-###-###-###-###-###-###-### 
def roll_dice(sides, num_rolls):
 total_rolls = 0
 roll_result = 0
 result_sum = 0
 for i in range(0, num_rolls):
 roll_result = randint(1, sides)
 print "Die #%d rolled %d" % (i+1, roll_result)
 result_sum += roll_result
 total_rolls += 1
 print "Total of %d dice rolls is: %d" % (num_rolls, result_sum)
 return None
def roll_again():
 global start
 start = raw_input("Roll again? ")
 return start
def promtp_change(cnt):
 global options_file, count_pos
 options_file.seek(count_pos, 0)
 pref_val_cnt = options_file.read(1) #The user's saved value for 'count'
 if int(cnt) == int(pref_val_cnt): #If they are the same, prompt to change dice
 temp_var_a = raw_input("Change dice? ")
 if temp_var_a == "y": #If yes
 clear()
 game() #Restart the game, by calling it again
 else:
 pass #Elsewise continue
 ###*###*###*###*###*###*###*###*### 
def main():
 initialize_variables() 
 open_file()
 initial_setup()
 menu()
 ###*###*###*###*###*###*###*###*### 
#----------------------Drop zone----------------------#
main()
#-----------------------ERRORS-EXPLAINED-----------------------#
# 0x001 : User's input was not expected.
# Possible reasons, most common:
# *Value not in possible choices
# *Expected str, input is int or vice-versa

Answer 1

def initialize_variables(): #Self Described
 global count, start, count_pos, initi_pos
 count = 0 
 start = ""
 count_pos = 2
 initi_pos = 0

• Do not use global variables, never. Functions are made to process input and give output. Your functions do not take input parameter. Change your code so that the functions will take input and there will not be global variables (or few of them).

• You have a nonsense comment: #Self Described. Comments like this one should be avoided because they just confuse the reader.

---

def open_file(): #Opens the options' file
 global options_file
 options_file = open("options.txt", "a+")
def close_file(): #Closes the option's file
 global options_file
 options_file.close()
 #return options_file.closed #Debugging purposes

These functions are meaningless because you can use:

with open('options.txt') as f:
 #do something

that will also handle closing automatically.

---

Error code: 0x001

What? You should print a human readable error message.

---

Please avoid:

print "Dice Rolling Simulator |"
print "-----------------------/"
print "1. Start rolling"
print "2. Change options"
print "3. Quit"

instead for better clarity you should print a multiline message declared at the start of the file:

MENU = """Dice Rolling Simulator |"
"-----------------------/"
"1. Start rolling"
"2. Change options"
"3. Quit"""
# other code
print MENU

• Veedrac: I'm not sure I agree about avoiding multiple lines of print; what matters is separating UI from logic.
• Caridorc: @Veedrac It is not about avoiding many print but about using the most constants possible. Anyway this is a minor point and as you say, it is much more important to learn to separate UI from logic.

Answer 2

Your code has no major issues, just a few problems to do with convention.

Documentation
Most of your functions are like this:

def clear(): #Usefull for clearing the terminal, should be cross-platform

When usually, you would use docstrings, usually saying what arguments the function takes and what it returns:

def clear():
"""Clears stdout, returns None"""

(The 'terminal' you reference is probably 'stdout')

Global Variables
They may seem useful at first, but should generally be avoided. A way to avoid this is passing variables into functions, for example:

def initial_setup(options_file, count_pos, initi_pos):
 stuff()

But how would you store those variables? You could use ambiguously indexed lists, or a dictionary, or you could use a class, except with a program this simple a dictionary is probably the way to go.

Error Messages
You print an error message with a hexadecimal value. This is not human readable. You should instead:

choice = raw_input("Your selection is: ")
try:
 choice = int(choice)
except ValueError:
 raise TypeError("Input must be of type int, not", type(choice).__name__)
if choice not in (1, 2, 3):
 raise ValueError("Your selection must be either 1, 2 or 3")

Recursion
When your program ends, it asks you if you want to restart it. To restart, it calls the menu() function again. This works in your situation, but it is good to know the dangers. Python has a max recursion depth, which means this will only work some times until it reaches max recursion depth and crashes. You could prevent this by putting the whole contents of the main() fuction in a while loop.

• Caridorc: Incredibly the documentation bit was exactly what I was writing in my answer... but you were faster posting.

Answer 3

I will try to add to the already existing answers.

I think that if you are up to coming to CR you won't badmouth PEP-8. If you do not know what it is, read it.

Imports should be on separated lines.

Two empty lines before a function declaration (one if it is inside a class).

#return options_file.closed #Debugging purposes

Good. But this should be production code, removing debugging code (not just by commenting out). Any serious development (you called this a program) should be done under VCS, in such environments you just delete code, don't comment it out.

###*###*###*###*###*###*###*###*###

Is this a fence to protect a function from the other functions?

#-----------------------ERRORS-EXPLAINED-----------------------#
# 0x001 : User's input was not expected.
# Possible reasons, most common:
# *Value not in possible choices
# *Expected str, input is int or vice-versa

Good, this way when the user sees 0x001 in the screen he/she e-mails you asking what the error is and then you either tell them how to edit the file or read it for them.

Short Python programs usually are "doable" without even a single global variable. Try it.

Someone above suggested classes. Don't go that way. Python classes are slow and fat and you just want to roll a dice, it is not really necessary to have classes here.

Last but not least, try to name functions with verbs or verbal phrases. It makes more sense to kill_foo_bar() than foo_bar_death().

Answer 4

Your imports break PEP 8. They should be something like

import os
import sys
import time
from random import randint

You often comment your functions like

def x(): # does a thing

Not only do I personally dislike end-of-line comments, these should be doc comments

def x():
 """Does a thing."""

Also, don't make comments like #Self Described.

You do screen constrol mostly by just running clear at opportune moments. This would be improved if you used a library built for this. Normally one would mention something terribly complicated like ncurses but I shall mention blessings, easily the nicest way to mess with the screen.

You are using global variables. Don't. A simple solution is just to pass these around as arguments. Note that as I made this change it became apparent that initialize_variables wasn't needed since it mostly initializes global constants.

Please wrap comments like

#Check the first value of the file, to determine if the user is running the Dice Simulator (DS from now on)

Your if here is formatted oddly:

if (options_file.read(1) == "0") :

You can just do

if options_file.read(1) == "0":

This loop won't work like you expect:

for i in range(1,4):
 time.sleep(0.5)
 print ".",

The terminal is buffered so you need to flush is between prints.

It seems like you're using options_file as an ad-hoc data store. You should look into structured approaches like JSON or databases. The easiest in this case, perhaps, would be shelve. This allows sweeping simplifications to the code.

Comments like

clear() #Clear the window

are useless. It's just repeating what's already there. Remove them.

Don't do decorations like:

 ###-###-###-###-###-###-###-###-### 

There's no need to return None from roll_dice, and no need to print the return value (which is None).

I don't get what this is:

#----------------------Drop zone----------------------#
main()
#-----------------------ERRORS-EXPLAINED-----------------------#
# 0x001 : User's input was not expected.
# Possible reasons, most common:
# *Value not in possible choices
# *Expected str, input is int or vice-versa

I suggest you remove the comments.

You don't need to write

else:
 pass

You should handle your files with a with statement, not manual closing. Unfortunately you're using Python 2, so you'll need contextlib.closing. open_file doesn't really do anything, so remove and inline it.

Your naming is subpar; consider

x = int(raw_input("The size of the dice(s) is: "))
y = int(raw_input("How many times will you roll it? "))

Why x and y?

You are using haphazard recursion to generate loops. Instead, define suitable functions to guide your loops and make functions self-contained where possible. For example, instead of using # Restart the game by calling it again, have an outer loop that calls promtp_change after the game has ended to allow it to restart. This should remove the need to call sys.exit from the functions (you shouldn't be doing that). Because the game seems a bit underspecified, I wasn't able to bring all behaviours across: what exactly is promtp_change meant to do? It seems to do nothing.

game has duplicated logic in the ifs. Move it outside of the ifs. This is also duplicated in menu, so extract it into a function.

I would move to new-style formatting ("{}".format instead of "%s" %).

In roll_dice, you don't need total_rolls since it's just num_rolls. You shouldn't give a default to roll_result; it just hides bugs. You don't need a 0 argument to range. In fact, I would write it as

rolls = [random.randint(1, sides) for _ in range(num_rolls)]
for i, roll in enumerate(rolls, 1):
 print "Die #{} rolled {}".format(i, roll)
print "Total of {} dice rolls is: {}".format(num_rolls, sum(rolls))

This all gives a much cleaner control flow and execution model, and the much less buggy code:

import random
import shelve
import sys
import time
from contextlib import closing
from blessings import Terminal
def initial_setup(options):
 if not options.get("has_run", False):
 options["has_run"] = True
 print "Welcome to the dice simulator"
 print "It seems like you are here for the first time."
 set_options(options)
 print "Done!",
 for i in range(1, 4):
 time.sleep(0.5)
 print ".",
 sys.stdout.flush()
 time.sleep(2)
def set_options(options):
 options["count"] = int(raw_input("After how many rolls will you change the dice? "))
def menu(term, options):
 while True:
 print "Dice Rolling Simulator |"
 print "-----------------------/"
 print "1. Start rolling"
 print "2. Change options"
 print "3. Quit"
 choice = int(raw_input("Your selection is: "))
 if choice == 1:
 game(term, options)
 return
 elif choice == 2:
 set_options(options)
 print(term.clear())
 else:
 alert_exit(choice == 3, sleep_for=3)
 return
def game(term, options):
 while True:
 die_size = int(raw_input("The size of the dice(s) is: "))
 die_count = int(raw_input("How many times will you roll it? "))
 start = raw_input("Roll the dice (y/n)? ")
 for count in range(options["count"]):
 print(term.clear())
 if start == "y":
 roll_dice(die_size, die_count)
 start = raw_input("Roll again? ")
 else:
 alert_exit(start == "n", sleep_for=2)
 return
 # What prompt_change would do... if it did something
 raw_input("Change dice? ")
def alert_exit(is_clean, sleep_for):
 if is_clean:
 print "Application will now exit."
 else:
 print "Invalid input. Error code: 0x001"
 print "Application will now exit."
 time.sleep(sleep_for)
def roll_dice(sides, num_rolls):
 rolls = [random.randint(1, sides) for _ in range(num_rolls)]
 for i, roll in enumerate(rolls, 1):
 print "Die #{} rolled {}".format(i, roll)
 print "Total of {} dice rolls is: {}".format(num_rolls, sum(rolls))
def main():
 term = Terminal()
 with closing(shelve.open("options.txt")) as options:
 initial_setup(options)
 print(term.clear())
 menu(term, options)
main()

However, this hasn't separated logic from UI (output), which is another important aspect of code quality. It also doesn't deal with invalid input (such as non-integers being passed to int).

• TheAngryAvocado: 'prompt_change'. With this fucntion, I ask the user if he wants to change dices after a number of rolls. For each roll, I add 1 to the variable 'count'. With calling 'prompt_change', I compare 'count' with the value on 'nth' bit of the file 'options.txt'. If they are equal, the user can change dice. Else, nothing happens. I hope I made it clear to you. Note that I've deleted every single comment, since they weren't commenting in any usefull way.

Answer 5

Just fyi .. There nothing wrong in doing main(), but another preferred way is

if __name__ =='__main__':
 main()
 .. OR ..
 initialize_variables() 
 open_file()
 initial_setup()
 menu()

http://effbot.org/pyfaq/tutor-what-is-if-name-main-for.htm