25
\$\begingroup\$

I've started coding about 3 weeks ago, just for fun and maybe practical uses later. My GF dared me to make a PGM that would replace this dice game where you roll two dice, one with body parts and one with actions, then resulting in stuff like 'kiss, lips'.

All I know is off of YouTube and sites such as Stack Overflow. I would appreciate if I could get tips and constructive criticism on how to code more efficient, clear, correct and maybe more 'pythonic'.

A bunch of notes are in there to clarify what I'm doing. As far as I've tested it is working correctly. As you will see there is a load/save function intended to store the game data to a text file.

def enterplayers():
    # Set players for the game
    global player1
    player1 = input('Enter player 1 name: ')

    global player2
    player2 = input('Enter player 2 name: ')

    global playerlist
    playerlist = [player1, player2]

    global player
    player = cycle(playerlist)

def enteractions():
    # Add actions to existing (or empty) list
    print("Add ACTIONS and enter 'q' when done")
    while True:
        nextitem = input("Add: ")
        if nextitem == 'q':
            break
        elif nextitem == '':
            continue
        else:
            nextitem = nextitem.lower()
            if ' ' in nextitem:
                nextitem = nextitem.replace(' ', '_')
            actionlist.append(nextitem)

def enterbp():
    # Add body parts to existing (or empty) list
    print("Add BODY PARTS and enter 'q' when done")
    while True:
        nextitem = input("Add: ")

        if nextitem == 'q':
            break
        elif nextitem == '':
            continue
        else:
            nextitem = nextitem.lower()
            if ' ' in nextitem:
                nextitem = nextitem.replace(' ', '_')
            bplist.append(nextitem)

def loaddata():
    #empty them out in case something there
    global bplist
    bplist = []

    global actionlist
    actionlist = []

    #Load each one in its correct list and players in each variable
    while True:
        loadfile = input('Enter file name to LOAD (including ".txt"): ')
        print()
        if loadfile == 'q':
            return # It was told to me that using this is baaadddd baaaaddddd; is that true?
        try:
            with open(loadfile,'rt') as file:
                for line in file:
                    line = line.split()

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

                    if 'bodypart:' in line:
                        line.remove('bodypart:')
                        newitem = ''
                        newitem = newitem.join(line)
                        bplist.append(newitem)

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

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

        except FileNotFoundError:
            print("That didn't work out! Probably file does not exist or wrong file location.")
            print("Try again, or enter 'q' to return to main menu.")
            print()


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

    print('Loaded actions:')
    for item in actionlist:
        print('-', item)
    print()

    print('Loaded body parts:')
    for item in bplist:
        print('-', item)
    print()

    print('Loading complete')
    print()


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

    global player
    player = cycle(playerlist)

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

    with open(savename, 'wt') as file:
        file.write('player1: ' + player1 + '\n')
        file.write('player2: ' + player2 + '\n')
        for item in actionlist:
            file.write('action: ' + item + '\n')
        for item in bplist:
            file.write('bodypart: ' + item + '\n')


    print()
    print('Successfully saved the following data to file', savename + ':')


    print('Player 1:', player1)
    print('Player 2:', player2)
    print()
    for item in actionlist:
        print('Action:', item)
    print()
    for item in bplist:
        print('Body part:', item)
    print()

def pickfirstplayer():
    # Check if user wants to pick randomly
    while True:
        print()
        randomplayer = input('Pick first player randomly? ')
        if randomplayer in ('no','yes'):
            break
        print('Please enter yes or no')
        print()

    # Pick randomly
    if randomplayer == 'yes':
        topickfrom = [player1,player2]
        nextplayer = random.choice(topickfrom)
        nextplayercheck = nextplayer
        print('The first player is ' + nextplayer + '!')

        # Here make sure that the firt playthrough is correct and not
        # player1 or player2 do something to himself
        nextplayer = next(player)
        if nextplayer != nextplayercheck:
            nextplayer = next(player)


    # Pick manually
    elif randomplayer == 'no':
        check = 0
        nextplayer = ''
        while check == 0:
            var = input('who is first player? ')
            if var not in playerlist:
                 print('That player is not playing!')
                 continue

            while var != nextplayer:
                nextplayer = next(player)
            check = 1
            print()

def menu():
    print()
    print('- - - - - - - - - - - -')
    print('   MAIN MENU')
    print()
    print('1. Start game')
    print()
    print('2. Enter player names')
    print('3. Add actions')
    print('4. Add body parts')
    print()
    print('6. Load data')
    print('7. Save data')
    print()
    print('8. Print instructions')
    print('9. Quit ')
    print('- - - - - - - - - - - -')
    print()

def instructions():
    print('Before starting the game you must set the following:')
    print('\t\t- Names of the players')
    print('\t\t- Actions (touch, kiss, etc.)')
    print('\t\t- Body parts')
    print()
    print('You can also just load a file with game data.')
    print()
    print('You do not need to capitalize, this is done automatically.')
    print()
    print('When this is done you can play the game!')
    print()
    print('Notice that when you use the load function,')
    print('  all previously loaded data will be removed.')
    print()



actionlist = []
bplist = []



import random
import time
import sys
from itertools import cycle



## 'actual code' starts here
while True:
    menu()
    try:
        menuchoice = int(input('Enter choice: '))

    except ValueError:
        print('Please enter a number!')
        continue


    print()

    if menuchoice == 1:

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

        pickfirstplayer()

        while True:
            # Set action and place randomly
            nextaction = random.choice(actionlist)
            nextplace = random.choice(bplist)

            # Print it
            print('Okay', nextplayer + ',')
            time.sleep(1)
            nextplayer = next(player)
            print()

            print(nextaction + ' ', end="", flush=True)
            time.sleep(1)
            print(nextplayer+"'s ", end="", flush=True)
            time.sleep(2)
            print(nextplace + '!')
            time.sleep(5)

            print()
            print()

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

            if anotherone == '':
                print()
                continue
            else:
                print('Returning to main menu')
                print()
                break

    elif menuchoice == 2:
        enterplayers()

    elif menuchoice == 3:
        enteractions()

    elif menuchoice == 4:
        enterbp()

    elif menuchoice == 6:
        loaddata()

    elif menuchoice == 7:
        savedata()

    elif menuchoice == 8:
        instructions()

    elif menuchoice == 9:
        quit()

    else:
        print('Please enter a valid number!')
        print()
\$\endgroup\$
21
\$\begingroup\$

First off, welcome to programming and Python. They are both awesome!

Ok, with this being your first foray into coding, instead of giving specific code improvements, I will give some high-level recommendations and then let you use them as you will.

Style

Python has an official style guide called PEP8 that defines the preferred way to style your code. It touches on variable naming conventions, proper white space, flow of code, etc. I strongly advise taking a look and trying to make your code fit what it says.

Classes

This program description is basically built for object-oriented programming (OOP). Classes are very easy to create in Python and provide simple ways to access similar data. Below is a skeleton Die class that can be used to represent one of your dice in your game:

class Die():
    def __init__(self, options=[]):
        # This is the function that is called when a new `Die` instance
        # is created. The `options` parameter is optional (denoted with the `=`).
        self.options = options 

    def __str__(self):
        # This 'magic method' is called when you used the `print` function.
        return ', '.join(self.options)

    def add_options(self, options):
        # Adds multiple options. HINT: use the following method.

    def add_option(self, option):
        # Add an option to the current list

    def roll(self):
        # Return one of its options based on a random number

From this point, using a Die is as easy as:

 >>>body_parts = Die(['arm', 'leg', 'nose'])
 >>>body_parts.add_options(['head', 'foot'])
 >>>body_parts.roll()
 'leg'
 >>>print(body_parts)
 arm, leg, nose, head, foot

Much of your code can be pulled into classes (Die as above, Game, Player, etc.). This is evident by how much you use the global keyword. Having to use a variable in many places implies a relationship between the variable and the functions it is used in. This relationship is one of the main ideas behind OOP.

Strings

You do a lot of print statements. That is fine. However, you can take out about 95% of them if you use triple-quote syntax. Essentially by using '''Some text''' Python keeps the structure of the string as it looks in the code. So your menu function can be scaled down from this:

def menu():
    print()
    print('- - - - - - - - - - - -')
    print('   MAIN MENU')
    print()
    print('1. Start game')
    print()
    print('2. Enter player names')
    print('3. Add actions')
    print('4. Add body parts')
    print()
    print('6. Load data')
    print('7. Save data')
    print()
    print('8. Print instructions')
    print('9. Quit ')
    print('- - - - - - - - - - - -')
    print()

to this:

def menu():
    print('''
- - - - - - - - - - - -
   MAIN MENU
1. Start game

2. Enter player names
3. Add actions
4. Add body parts

5. Load data
6. Save data

7. Print instructions
8. Quit
- - - - - - - - - - - -
''')

Also, you have some statments that use strings like this:

file.write('player1: ' + player1 + '\n')

A simple way of writing this is using the format() function provided by Python:

file.write('player1: {}\n'.format(player1))

This way we don't have to worry about all of the string concatenation that is happening in the old statement.

Miscellaneous Thoughts

Here are some random improvements I can see:

  1. replace()

    There is no need to check for a substring in a string before replacing. If there are none, replace() will handle it gracefully:

    >>>'Hello Planet!'.replace('Planet', 'World')
    Hello World!
    >>>'Hello World!'.replace('Planet', 'World')
    Hello World!
    
  2. 'Infinite' Loops

    While I love while True loops in the right circumstances, you do not need most (if not all) of them. Take you while-loop in enterbp:

    def enterbp():
        # Add body parts to existing (or empty) list
        print("Add BODY PARTS and enter 'q' when done")
        while True:
            nextitem = input("Add: ")
    
            if nextitem == 'q':
                break
            elif nextitem == '':
                continue
            else:
                nextitem = nextitem.lower()
                if ' ' in nextitem:
                    nextitem = nextitem.replace(' ', '_')
                bplist.append(nextitem)
    

    This could be simplified to:

    def enterbp():
        # Add body parts to existing (or empty) list
        print("Add BODY PARTS and enter 'q' when done")
    
        nextitem = input("Add: ")
        while nextitem != 'q':
            if nextitem == '':
                continue
    
            bplist.append(nextitem.lower().replace(' ', '_'))  
    
            nextitem = input("Add: ")
    
  3. Using lists

    lists are awesome. You could simply use a list to implement your menu:

    # This is a list of functions.
    menu_options = [play, enterplayers, enteractions, enterbp,
                    loaddata, savedata, instructions, quit]
    
    choice = input('What do you choose: ')
    
    # Finds the correct function, then calls it
    menu_options[choice-1]()
    

There are other things, but I think this is a good place to draw a breath as I've already innudated you with information. :D

\$\endgroup\$
  • \$\begingroup\$ Great answer!I'm reworking my code now. would it also be better to replace print('Player 1:', player1) with print('Player 1: {0}'.format(player1)) or would you consider that useless(since now the code is longer)? Thanks for taking the time! \$\endgroup\$ – LuukV May 21 '14 at 14:56
  • 2
    \$\begingroup\$ Personally, I would still use the format function as its simply more Pythonic. As for performance, I've found resources that say concatenation is faster and ones that say string substitution is faster. \$\endgroup\$ – BeetDemGuise May 21 '14 at 15:27
  • \$\begingroup\$ Thanks, I'm still kind of figuring out the classes (__init__ and so on), but I'll manage with existing tutorials. So far this has definitely improved my code a lot, very nice. \$\endgroup\$ – LuukV May 21 '14 at 20:10
  • \$\begingroup\$ Yeah, at first class syntax and thinking can be a little odd. If you have any additional questions go ahead and ask. \$\endgroup\$ – BeetDemGuise May 22 '14 at 11:53
  • \$\begingroup\$ Could you shed some light on this: I tried putting my menu in a list as you said. But now that means to me that I will need to put almost my entire main block in a functions called play(). Then I have almost no global variables and my other functions can't use them unless I start going crazy with global. But I know thats not what I'm supposed to do. So now I'm stuck here with all these variables that will only work within that one function. Or am I looking at thing wrongly? \$\endgroup\$ – LuukV May 23 '14 at 0:42
13
\$\begingroup\$

Many things need improvement I'll edit this answer as the day goes

print() and strings.

You are using print redundantly. consider the menu function. You can use block text with either """ """ or ''' '''.

With that menu function could be just one print statement. Or just store it as a global variable like so

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

    2. Enter player names
    3. Add actions
    4. Add body parts

    6. Load data
    7. Save data

    8. Print instructions
    9. Quit 
----------------------------
'''

and print the menu simply

print(MAIN_MENU)

You are also using print()print() to print new lines. Just use print('\n\n') '\n' is the common new line character in almost every programming language.

Use a Main Block

You start your program like this

## 'actual code' starts here
while True:

Put your main block in a main block like this

if __name__ == '__main__':
    while True:

global

The global keyword is one of those things in python that smells. Avoid it as much as possible. In your main block, it's not plainly clear what 'enterplayers' does as it takes no arguments nor returns any value. Yet somehow its modifying your local variables. It should return the players entered.

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

...
if menuchoice == 2:
    players = enterplayers()

Actually players isn't declared anywhere in your main block which is even more mysterious. One should be able to just look and that block and know where players is defined.

Re-implementing switch

In your main block you essentially re implement a C style switch block for your menu. This also smells particularly bad in python.

Instead create a menu dictionary where the values are the actual functions you call and simply call them with menuchoice

menu_actions = {1: play_game, 2: enterplayers, 3: enteractions, ... }
menu_actions[menuchoice]()

Note that in menu_actions we store the functions themselves without calling them. This is one of my favorite features of python: you can get what would be a delegate or pointer to function just by using the name of the function without (). This is because all functions are objects in python. Here's an example

This works well as your code is because all of these functions return nothing and take no arguments. If you follow my suggestion for enterplayers you will need to implement this differently as I suggested that enterplayers should return something.

Implementing both of my suggestions gets a bit more tricky. I'm not going to claim to know the best way to do it and if it gets too complicated the simple switch or global call might be the better way.

Passing parameters

The above two liner works because all of your functions take the exact same parameters, None. If they took the exact same non void parameters the second line would like this.

menu_actions[menuchoice](COMMON_PARAMETERS)

Chances are that will not be the case and each function will need unique parameters.
You could implement them using functools.partial. Make the menu_actions a dictionary of partial functions,

menu_actions = {1: functools.partial(play_game, some_arg=players), 
    2: enterplayers,  # eg if this function takes no args
    3: functool.partial(enteractions, some_arg=some_param),
    4: ...
    }
menu_actions[menuchoice]()

This won't work if the values of parameters you pass change between the definition of menu_actions and the calling of the partial function. You would need to re-declare menu_actions every time some parameter changes. That makes this a messy solution.

\$\endgroup\$
  • \$\begingroup\$ Thanks for helping out, I will try and keep up with all you mention(ed) in a separate file and maybe resubmit in a new post when done. \$\endgroup\$ – LuukV May 21 '14 at 14:13
  • \$\begingroup\$ On the last part (dictionary for menu), how would that work if my function needs parameter? I understand from this that I don't add () so that would mean there is no space to add parameters needed for the function. Or can I just add them anyway if I need them? \$\endgroup\$ – LuukV May 21 '14 at 20:14
  • \$\begingroup\$ FYI, re your part on global variables. I tried players = cycle(player1, player2) but it gives an error due to cycle() only taking one argument. I changed it to a list like so players = cycle([player1, player2]) and this worked. Thanks again for the help. \$\endgroup\$ – LuukV May 22 '14 at 21:39
  • \$\begingroup\$ edit made. cycle requires an iterable to convert. \$\endgroup\$ – cheezsteak May 23 '14 at 13:21

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.