4
\$\begingroup\$

I think this code has far to many if else clauses and not enough abstraction via functions. I would like to break the code down into a series of small functions, or methods if i decide to go OOP, but I'm not sure of the best way to do so. If you brilliant Pythonista's could guide me in the write direction I would appreciate it.

Here is the GitHub repo if you would like to fork it repo

'''
script to determine the outcome(s) of any number of dice rolls
By Andrew Smith (2015)
'''

from random import randint

def dice_roller(dice, rolls, sides):
    '''
    Add doc string here.
    '''
    show_di = '|   |'
    show_di_1 = '| * |'
    show_di_2 = '|* *|'
    show_di_3 = '|***|'
    show_di_t = ' ___ '
    show_di_b = ' --- '
    roll_counter = 0
    di_counter = 0

    while roll_counter < rolls:
        roll_counter += 1
        di_counter = 0
        print " "

        while di_counter < dice:
            di_counter += 1
            num_side = randint(1, sides)
            print "Roll %r, Di %r: %r" % (roll_counter, di_counter, num_side)
            if num_side <= 6: # print di symbol if di is six-sided
                if num_side == 1:
                    print show_di_t
                    print show_di_1
                    print show_di
                    print show_di_b
                elif num_side == 2:
                    print show_di_t
                    print show_di_1
                    print show_di_1
                    print show_di_b
                elif num_side == 3:
                    print show_di_t
                    print show_di_1
                    print show_di_2
                    print show_di_b
                elif num_side == 4:
                    print show_di_t
                    print show_di_2
                    print show_di_2
                    print show_di_b
                elif num_side == 5:
                    print show_di_t
                    print show_di_2
                    print show_di_3
                    print show_di_b
                elif num_side == 6:
                    print show_di_t
                    print show_di_3
                    print show_di_3
                    print show_di_b
    print " "
#-------------------------------------------------------------------------------

TITLE = """
 __________________
| Dice Roller v1.1 |
| by Andrew Smith  |
|__________________|
"""

MENU = """
Enter the number of dice to roll, the number
of rolls, and the number of sides per di in the
following format: #dice #rolls #sides

--- or ---

Select from the following presets:

a. 3 man attack (Risk)
b. 2 man attack or defend (Risk)
c. 1 man attack or defend (Risk)

--- or ---

Type 'menu' to repeat this menu

--- or ---

Type 'quit' to quit
        """

def main():
    '''
    Explain this function.
    '''
    print TITLE
    print MENU
    again = 0

    while again != 1: # prompts the user until 'quit' is entered
        roll_choice = raw_input("> ")
        if roll_choice.isalpha(): # if input starts with a letter
            if roll_choice == 'a':
                dice_roller(3, 1, 6)
            elif roll_choice == 'b':
                dice_roller(2, 1, 6)
            elif roll_choice == 'c':
                dice_roller(1, 1, 6)
            elif roll_choice == 'menu':
                print MENU
            elif roll_choice == 'quit':
                again = 1
            else:
                print "Invalid choice.\n"

        elif roll_choice[0].isdigit(): # if input starts with a number
            choices = roll_choice.split()
            size = len(choices)
            if size == 3:
                one = int(choices[0])
                two = int(choices[1])
                three = int(choices[2])
                dice_roller(one, two, three)
            else:
                print "Invalid Choice.\n"
        else:
            print "Invalid choice.\n"

if __name__ == '__main__':
    main()
\$\endgroup\$
  • \$\begingroup\$ while counter < N: counter += 1 is something of a red flag :) why not a for loop? \$\endgroup\$ – Eevee Nov 20 '15 at 8:23
4
\$\begingroup\$

Use unpacking

        choices = roll_choice.split()
        size = len(choices)
        if size == 3:
            one = int(choices[0])
            two = int(choices[1])
            three = int(choices[2])
            dice_roller(one, two, three)
        else:
            print "Invalid Choice.\n"

Becomes:

dice_roller( * map(int, roll_choice.split()) )

Adding error handling it is:

 try:
     dice_roller( * map(int, roll_choice.split()) )
 except Exception as e:
     print("Invalid choice:" + e)

The * operator just goes from a list to free floating arguments and is called the unpacking operator. You use it to pass a list as argument to a function that takes separate arguments.

Naming in full

You should write the names of your variables in full to make them more understandable at first glance. show may be omitted. For example:

  • show_di_1dice_1
  • show_di_bdice_bottom

A dictionary

A dictionary will allow you to avoid the first if elif chain easily. How is left as an exercise for the reader.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Here is some general style comments, related to the previously given advice of reading and adhering to PEP8:

  • Use docstrings, but let them have meaning – To have docstrings and comments can help understanding code, but stuff like 'explaing this function' or 'add doc string here' is a little annoying
  • You have many good variable and function names – You've found that in Python one uses snake_case for variable and function names, and that is good. You've also picked up on CONSTANTS using capital letters.

    You could hovewer move your constants to a group closer to the top of the file. Normal sequence is typical: imports, constants, functions (and/or classes), main function called by the if __name__ ... construct, like you already do.

  • Change to newer style print function – From Python 3 you'll have to use print as a function, but one can start doing this already to make the transistion easier. On the same tangent one can start using the new style of print formatting. This changes you print statements to stuff like print('Roll {}, Dice {}: {}'.format(roll_counter, di_counter, num_side))
  • Make functions instead of big if ... elif chains – Within dice_roller it would be better to extract all the print show_... into a separate function. Whenever you find your self copy-pasting code, think functions. In this case you take it all the like in my question (inspired by this question), or you do a simpler variant like:

    def print_dice(roll):
        no_eyes = '|   |'
        one_eye = '| * |'
        two_eyes = '|* *|'
        three_eyes = '|***|'
        top_line = ' ___ '
        bottom_line = ' --- '
    
        print(top_line)
        # First line
        if roll == 6:
            print(three_eyes)
        elif roll >= 4:
            print(two_eyes)
        else:
            print(one_eye)
    
        # Second line
        if roll == 1:
            print(no_eyes)
        elif roll = 2:
            print(one_eye)
        elif roll == 6:
           print(three_eyes)
        else: 
            print(two_eyes)
    
        print(bottom_line)
    

    Especially note how I moved the printing of the top and bottom line out on the outside of the if blocks, and that I also consolidated the rolls which prints the same number of eyes. And that I've written out the variable names to two_eyes instead of show_di_2. The latter kind of implicates that the variable is showing something, which could be true if it was a function, but not as a variable.

  • Avoid flag variable, if possible – Sometimes you need to do stuff like while again != 1, but the first change would be to actually use while again where normally again = True, but even better here would be to use while True:, and then do break when roll_choice == 'quit'.

  • Avoiding one-time temporary variables – Two examples in your code, is the again variable, but another one is the size = len(choices) followed by if size == 3:. It could be good to do the len() once, if you use the result multiple times, but if only used once, it is usually better to be explicit and only do the if len(choices) == 3:. (Even better is to use the alternate unpacking suggested in another answer)
| improve this answer | |
\$\endgroup\$

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.