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

Question

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

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

        while True:
            additem = input('Add: ')
            if additem.lower() == 'q':
                break
            elif additem == '':
                continue
            else:
                self.options.append(additem.lower().replace(' ', '_'))

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


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


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


def load():
    # Load each one in its correct list and players in each variable
    # What is already in the lists will still be there
    while True:
        loadfile = input('Enter file name to LOAD (including ".txt"): ')
        print()
        if loadfile.lower() == 'q':
            return  # It was told to me that using return like this is bad, true?
        try:
            with open(loadfile, 'rt') as file:
                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:
                        print("ERROR: item", errormessage, "not loaded")

        except FileNotFoundError:
                        print("That didn't work out! File does not exist or wrong file location.")
            print("Did you forget to add .txt?")
            print("Try again, or enter 'q' to return to main menu.")
            print()

        break

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

    # Tell user what is loaded
    print('Loaded players:')
    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('Loading complete')
    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:
            print('Please enter a number!\n')
            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:
            print('Please enter a number!\n')
            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])


MAIN_MENU = '''
- - - - - - - - - - - -
   MAIN MENU
1. Start game

2. Enter player names
3. Show dice

4. Add actions
5. Add body parts
6. Remove actions
7. Remove body parts

8. Load data
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:
        print(MAIN_MENU)
        menuchoice = input('Choice: ')

        if menuchoice == '1':
            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('\nReturning to main menu')
                    print()
                    break

        elif menuchoice == '2':
            players = setplayers()
            player1 = next(players)
            player2 = next(players)

        elif menuchoice == '3':
            printdice()

        elif menuchoice == '4':
            addactions()

        elif menuchoice == '5':
            addbp()

        elif menuchoice == '6':
            removeactions()

        elif menuchoice == '7':
            removebodyparts()

        elif menuchoice == '8':
            load()

        elif menuchoice == '9':
            save()

        elif menuchoice.lower() == 'i':
            print(INSTRUCTIONS)

        elif menuchoice.lower() == 'q':
            quit()

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

Answer 1

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.

Answer 2

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 :

Your code becomes :

            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:
                        print("ERROR: item", key, "not loaded")

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

Answer 3

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:
    players, dice = load(filename)
except IOError:
    print("Couldn't load that file.")

---

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)

    def add_option(self, option):
        self.options.append(option)

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

    ...

---

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

class Game:

    def main_menu(self):
        while True:
            ...

    def load_from_file(filename):
        ...
        self.players = players
        self.dice = dice

    def play_round(self):
        ...

    def save_to_file(filename):
        ...

Then your main loop is simply:

game = Game()
game.main_menu()