# Spin-the-bottle-like game, follow-up

I've posted this some time ago. Here is the new version that I made based on all the comments that were made. Another look would be much appreciated, especially on:

• Getting rid of the global statements

• I read about the use of the return statement which is bad practice in some situations (see code). Is that correct?

• The if elif else way of coding the menu (at the bottom) which was proposed to be put in a list and then calling the list items. I did not manage to get the first option of actually playing the game to work.

import random
import time
from itertools import cycle

class Die:
def __init__(self, name):
self.name = name
self.options = []
self.pick = 0

print('Enter {0} and press enter. Enter "q" when done.\n'.format(self.name))

while True:
break
continue
else:

def roll(self):
self.pick = random.choice(self.options)

print('Add actions to the dice. Enter "q" when done')
while True:
if nextitem == '':
continue
elif nextitem.lower() == 'q':
break
else:
actionDie.options.append(nextitem.lower().replace(' ', '_'))

print('Add body parts to the dice. Enter "q" when done')
while True:
if nextitem == '':
continue
elif nextitem.lower() == 'q':
break
else:
bpDie.options.append(nextitem.lower().replace(' ', '_'))

# Load each one in its correct list and players in each variable
# What is already in the lists will still be there
while True:
print()
return  # It was told to me that using return like this is bad, true?
try:
for line in file:
errormessage = line
line = line.split()

if 'action:' in line:
line.remove('action:')
newitem = ''
newitem = newitem.join(line)
actionDie.options.append(newitem)

elif 'bodypart:' in line:
line.remove('bodypart:')
newitem = ''
newitem = newitem.join(line)
bpDie.options.append(newitem)

elif 'player1:' in line:
line.remove('player1:')
newitem = ''
newitem = newitem.join(line)
global player1
player1 = newitem

elif 'player2:' in line:
line.remove('player2:')
newitem = ''
newitem = newitem.join(line)
global player2
player2 = newitem

else:

except FileNotFoundError:
print("That didn't work out! File does not exist or wrong file location.")
print("Did you forget to add .txt?")
print()

break

global players
players = cycle([player1, player2])

# Tell user what is loaded
print('-', player1)
print('-', player2)
print()

print('Action die now contains:')
for item in actionDie.options:
print('-', item)
print()

print('Body parts die now contains:')
for item in bpDie.options:
print('-', item)
print()

print()

# Set list to cycle through (since enterplayers() was not used)
global playerlist
playerlist = [player1, player2]

global player
player = cycle(playerlist)

def printdice():
print('The body part die contains the following:')
for item in bpDie.options:
print('\t- {0}'.format(item))

print()

print('The actions die contains the following:')
for item in actionDie.options:
print('\t- {0}'.format(item))

def removeactions():
print("Removing from the actions die.\n")
while True:
print('The action die currently contains:')
for index, item in enumerate(actionDie.options):
print('{0} - {1}'.format(index + 1, item))
print()
try:
indexnr = int(input("Type the number of the item you wish to remove or enter '0' to quit: "))
except ValueError:
continue

if indexnr == 0:
break
indexnr -= 1

try:
actionDie.options.remove(actionDie.options[indexnr])
except IndexError:
print('Invalid number!\n')
continue

def removebodyparts():
print("Removing from the body parts die.\n")
while True:
print('The body parts die currently contains:')
for index, item in enumerate(bpDie.options):
print('{0} - {1}'.format(index + 1, item))
print()
try:
indexnr = int(input("Type the number of the item you wish to remove or enter '0' to quit: "))
except ValueError:
continue

if indexnr == 0:
break
indexnr -= 1

try:
bpDie.options.remove(bpDie.options[indexnr])
except IndexError:
print('Invalid number!\n')
continue

def save():
print()
print("Save including '.txt', any existing file will be overwritten!\n")
while True:
savename = input('SAVE to what file? ')
if '.txt' in savename:
break
print("Don't forget .txt!\n")

with open(savename, 'wt') as file:
try:
file.write('player1: {0}\n'.format(player1))
file.write('player2: {0}\n'.format(player2))
except NameError:
pass

for item in actionDie.options:
file.write('action: {0}\n'.format(item))

for item in bpDie.options:
file.write('bodypart: {0}\n'.format(item))

print('\nSuccessfully saved the following data to file "{0}":'.format(savename))
try:
print('Player 1:', player1)
print('Player 2:', player2)
print()
except NameError:
pass

for item in actionDie.options:
print('Action:', item)
print()
for item in bpDie.options:
print('Body part:', item)
print()

def setplayers():
global player1
player1 = input('Enter player 1 name: ')
global player2
player2 = input('Enter player 2 name: ')
return cycle([player1, player2])

- - - - - - - - - - - -
1. Start game

2. Enter player names
3. Show dice

6. Remove actions
7. Remove body parts

9. Save data

i. Print instructions
q. Quit
- - - - - - - - - - - -
'''

INSTRUCTIONS = '''
Before starting the game you must set the following:
- Names of the players
- Actions (touch, kiss, etc.)
- Body parts

You can also just load a file with game data.

You do not need to capitalize, this is done automatically.

When this is done you can play the game!

Notice that when you use the load function,
all previously loaded data will be removed.

'''

bpDie = Die('body part')
actionDie = Die('action')
player1 = ''
player2 = ''

if __name__ == '__main__':
while True:

if player1 == '' or player2 == '':
print("\nPlayer name(s) not set!\n")
time.sleep(2)
continue

if not actionDie.options:
print("\nNo actions in die!\n")
time.sleep(2)
continue

if not bpDie.options:
print("\nNo body parts in die!\n")
time.sleep(2)
continue

print('- - - - - - - -')
print('STARTING GAME!')

while True:
nextplayer = input('Enter name of first player or press enter to pick randomly: ')

if nextplayer in (player1, player2):

# Set "next(players)" to the correct one (the person that will come after the first player)
nextplayercheck = nextplayer
nextplayer = next(players)
if nextplayer != nextplayercheck:
nextplayer = next(players)
break

elif nextplayer == '':
topickfrom = [player1, player2]
nextplayer = random.choice(topickfrom)
nextplayercheck = nextplayer
print('\nThe first player is {0}!'.format(nextplayer))

# If the next player is not next in the cycle(), then sets it so.
nextplayer = next(players)
if nextplayer != nextplayercheck:
nextplayer = next(players)
break

else:
print('That person is not playing!')

# START PICKING AND RETURNING THE ITEMS HERE
while True:
actionDie.roll()
bpDie.roll()

print('\nOkay {0}\n,'.format(nextplayer))
time.sleep(1)
nextplayer = next(players)

print(actionDie.pick + " ", end="", flush=True)
time.sleep(1)
print(nextplayer + "'s ", end="", flush=True)
time.sleep(1)
print(bpDie.pick, end=' ', flush=True)
time.sleep(1)
print(str(random.randrange(1, 11)) + ' times!')
time.sleep(1)
print('\n\n')

anotherone = input('Press enter for another, type anything to quit to main menu.')

if anotherone == '':
print()
continue
else:
print()
break

players = setplayers()
player1 = next(players)
player2 = next(players)

printdice()

removeactions()

removebodyparts()

save()

print(INSTRUCTIONS)

quit()

else:
print('Please pick a valid number or letter\n')

• Function names should be lowercase_with_underscores (PEP 8). And spell out the name of addbp as add_body_parts, to match remove_body_parts. Jun 2 '14 at 16:49

First off, your code is looking a lot better than what I remember seeing it before. You've taken a nice step towards good code.

Now onto my suggestions:

1. The functions addactions and addbp are identical to Die.add. So instead of calling addactions and addbp in your menu, simply call actionDie.add() and bpDie.
2. Much like my point above, removeactions and removebp are identical. A quick fix would be to pass a Die to the function and operate on that Die:

def remove(die):
print("Removing from the {} die.\n".format(die.name))
while True:
# Remove stuff


A much better solution though is to bring that function into the Die class. The remove functionality is something that you need for each Die so why not put it in the Die class?

class Die(object):
def __init__(self, name, pick):
# Stuff

def remove(self):
# Put remove code here
print("Removing from the {} die.\n".format(self.name))


As a quick aside about the remove functions, you each time in the while loop you are printing the entire options list. This seems like it could clog up the screen really fast if the user doesn't type in many numbers.

3. Since the Die class has only one internal list, look into using the __getattr__ and __setattr__ magic methods. These allow for you to using indexing syntax on your object:

class Foo(object):
def __init__(self, items):
self.items = items

def __getattr__(self, index):
return self.items[index]

def __setattr__(self, index, val):
self.items[index] = val

>>> foo = Foo(['a', 'b', 'c'])
>>> foo[1]
'b'
>>> foo[0] = 'test'
>>> foo.items
['test', 'b', 'c']


I have other suggestions as well, but they all seem to fall under one category: think about what you want (and need) the Die class to do. What functionality does a Die have to have?

Thinking about this, it would be nice to have the Die:

• Print itself
• Remove items from its list
• Add items to its list

You already do most of this functionality, however its done to the both Die specifically. You can simplify your code by bringing that functionality into a single place that, eventually, both Die can access.

Quick answer but the part in the with open(loadfile, 'rt') as file: block definitly looks wrong to me. Here are a few suggestions.

You shouldn't use the same variable for different purposes :

newitem = ''
newitem = newitem.join(line)
some_function(newitem)


What is newitem ? Is it a separator or a concatenation of values ? You can simply write this :

some_function(''.join(line))


Also, the way line is sometimes a string sometimes a list can lead to confusion.

If my understanding is correct, you have a list of looking like this : "some_kind_of_key: some kind of values". At the moment, the issue is that you are looking for the key on the whole line so that if you were to be fed a line like "bodypart: action:", this would be handled as if it was the file "action: bodypart:" which is very confusing. What you need to do is to perform some processing of the first item of the list :

            for str_line in file:
list_line = str_line.split()
if list_line:  # nothing to do for empty lines (?)
key = list_line[0]
values = ''.join(list_line[1:])
if key == 'action:':
actionDie.options.append(values)
elif key == 'bodypart:':
bpDie.options.append(values)
elif key == 'player1:':
global player1
player1 = values
elif key == 'player2:':
global player2
player2 = values
else:


There are many other things to change but I'll leave this to others.

Getting rid of global is a simple matter of learning about function arguments and return values. For instance, your load function could be refactored to:

def load(filename):
...
return players, dice


and called:

try:
except IOError:


Your file format is essentially csv with ':' instead of ',', so you could look into parsing with the csv module.

Hard-coding the number of players to two is unnecessary and actually makes your code more complex (though potentially popular with your GF!) In the text file, tag each simply as:

player:Alice
player:Bob
player:Colin


then when parsing:

part, value = line.strip().split(':')
if part == 'player':
players.append(value)


Similarly, you are limiting the format to two specifically-named dice. Instead consider:

die:body parts
side:legs
side:arms
die:actions
side:tickle


Much more of the functionality could be in the Die class, for example:

class Die:

def __init__(self, name, options=None):
if options is None:
options = []
self.name = name
self.options = options

def __str__(self):
data = ["The {0} die contains: ".format(self.name)]
for number, option in enumerate(self.options, 1):
data.append("{0}. {1}".format(option, number))
return "\n".join(data)

self.options.append(option)

new_option = input("Enter new option for {0}: ".format(self.name))

...


Finally, consider a Game class for the rest, e.g.:

class Game:

while True:
...

...
self.players = players
self.dice = dice

def play_round(self):
...

def save_to_file(filename):
...


Then your main loop is simply:

game = Game()