8
\$\begingroup\$

I wrote my first Python program today after working through a few books. I decided to build a group initiative tracker for our D&D group and was hoping to get some feedback. Critiques on code efficiency, formatting, suggestions, etc would be appreciated. My plan is to eventually take this from a command-line application to a django or flask web app, or maybe an iOS app.

print('Welcome to Initiative Tracker!')
while True:
    try:
        loop_count=int(input("\nHow many PC's and NPC's total? "))
        loop_start = 0
        initiative_tracking = {}
        while loop_start < loop_count:
            player_name = input('What is the Players name? ')
            player_initiative = int(input('What is ' + player_name + ' initiative? '))
            if player_initiative in initiative_tracking:
                initiative_tracking[player_initiative].append(player_name)
            else:
                initiative_tracking[player_initiative] = [player_name]
            loop_start += 1
            continue
        print('\nYour initiative order is: ')
        for key in sorted (initiative_tracking.keys(), reverse=True):
            print(str(key) + ': ' + ', '.join(initiative_tracking[key]))
        break
    except ValueError:
        print("Sorry I didn't catch that. How many PC and NPC total? ")
\$\endgroup\$
  • \$\begingroup\$ It looks like your else block is not indented correctly. Would you mind fixing or clarifying? The easy way to format the code is just paste it in, highlight it, and click the {} button or hit Ctrl-k. \$\endgroup\$ – zondo Jul 9 '16 at 4:23
  • \$\begingroup\$ Got it. Thanks for the tip I'll make sure to do that in the future. \$\endgroup\$ – Alexander Jul 9 '16 at 4:47
  • \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 Jul 10 '16 at 15:41
  • \$\begingroup\$ Got it. Appreciate the update I posted a self answer below per the linked article. Thanks! \$\endgroup\$ – Alexander Jul 10 '16 at 15:49
10
\$\begingroup\$

I see that you have some error handling for int(input(...)). I would change it, however. Let's say that the user types a valid number of PC's, but does not type a valid player initiative. That would trigger a ValueError, and your except block would ask again how many PC's. Why should it restart the whole process for one little mistake? You should have different try blocks for each of those. Also, a try block should include only the code that you expect might throw an error. If some other code throws an error, you want to know about it.

For the first use of int(input(...)), I would just take it out completely. Why should the user need to specify how many PC's? Why not just keep going until he runs out and then hit Enter on a blank line? That can be done quite simply with iter(). Usually, iter() is given an iterable, but if it is given two arguments, the first argument is a callable (usually a function) that returns a value. (In our case, we use lambda: input(...)). The second argument is the stopping point. iter() will keep calling that function and yielding the result until it returns the second argument. Therefore, we give it '', and we get all of the user's inputs until he types nothing in a line.

The second use of int(input(...)) is when you are retrieving the player initiative. It should have its own try-except block to make sure the input is an integer. I would then put that block inside of its own while loop so that the program keeps asking until it gets a valid answer.


Instead of a chain of string concatenations, use string formatting:

player_initiative = input('What is {} initiative? '.format(player_name))

and

print('{}: {}'.format(key, ', '.join(initiative_tracking[key])))

A loop is called that because it loops. You don't need to tell it to keep looping. It will keep looping until it is told to stop. Sometimes it is stops because a condition is not met, but sometimes it stops because it encounters a break. You should use continue only if you want to skip the code beneath and jump straight to the beginning of the loop. It really doesn't make sense to put continue at the end of the loop.


The argument that is given to sorted() has only one requirement: that it is iterable. To test if a type of object is iterable, go into a Python shell and create an instance of it. Then, do for item in my_object: print(item). If there is an error, your object is not iterable. Otherwise, you will see what happens when iterates. For a dictionary, you get all of the keys. Therefore, sorted(initiative_tracking.keys(), reverse=True) can be simplified to sorted(initiative_tracking, reverse=True).


Full code:

print('Welcome to Initiative Tracker!')

initiative_tracking = {}
for player_name in iter(lambda: input("What is the player's name? "), ''):
    player_initiative = input('What is {} initiative? '.format(player_name))
    while True:
        try:
            player_initiative = int(player_initiative)
            break
        except ValueError:
            player_initiative = input('Please enter an integer: ')

    if player_initiative in initiative_tracking:
        initiative_tracking[player_initiative].append(player_name)
    else:
        initiative_tracking[player_initiative] = [player_name]

print('\nYour initiative order is: ')
for key in sorted(initiative_tracking, reverse=True):
    print(str(key) + ': ' + ', '.join(initiative_tracking[key]))
\$\endgroup\$
  • \$\begingroup\$ The call to iter with 2 arguments is rather unusual. Mind adding a bit of explanations about how you use it and why? \$\endgroup\$ – Mathias Ettinger Jul 9 '16 at 7:13
  • \$\begingroup\$ @MathiasEttinger: Done. See the second paragraph. \$\endgroup\$ – zondo Jul 9 '16 at 12:12
  • \$\begingroup\$ I wouldn't say that you'll get an error from printing a generator, just unexpected behavior: def one(): yield 1 x = one() print(x) gives <generator object one at 0x10874deb8>. \$\endgroup\$ – Alec Jul 9 '16 at 14:40
  • 1
    \$\begingroup\$ @Alexander: I just looked up site:stackoverflow.com python lambda and got some pretty good results. The third one, seemed particularly useful. I also saw mentioned this page which also seems fairly easy to understand. \$\endgroup\$ – zondo Jul 9 '16 at 20:44
  • 1
    \$\begingroup\$ @Alexander: You could do that. I might add it in the input line: What is the player's name? (Hit Enter to end) '. I'm not sure what has the best UX. Maybe try them both out on your friends and/or family to see what they think. \$\endgroup\$ – zondo Jul 10 '16 at 3:26
4
\$\begingroup\$

Nice first script! I would suggest a few changes:

  1. Your try/ except block has too much in its scope. If you isolate out the part that will give you a ValueError (namely, trying to assign an int to an inappropriate input type), you can manage the event more precisely. I've edited it so that each case is tested separately. You could generalize the question of assigning an input to an integer if you preferred by using a function; I'd recommend that approach if you're repeatedly taking input in this way.
  2. By breaking your one block of code into functions, you will have more readable and more portable code. The if __name__ == '__main__ conditional at the end enables you to import these functions in another script (without automatically executing the main function) and reuse them should you so desire. Furthermore, you can use recursive function calls within the input functions to catch errors as they occur; that way you don't have to start over if something is entered incorrectly.
  3. I also took out the while loop and replaced it with a for loop. This way you'll be less likely to get caught in an infinite loop inadvertently, and you can also use the iterated value to numerate the players one-by-one when you ask for their name.

Here's the revised code:

def get_players():
    "function to get number of players, tries until successful"
    reply = input("\nHow many PC's and NPC's total? ")
    try:
        loop_count = int(reply)
    except ValueError:
        print('Sorry, I didn\'t get that. Try using an integer.')
        return get_players()
    return loop_count

def get_initiative(player_name):
    "function to get initiative, tries until successful"
    reply = input('What is ' + player_name + '\'s initiative? ')
    try:
        player_initiative = int(reply)
    except ValueError:
        print('Sorry, I didn\'t get that. Try using an integer.')
        return get_initiative(player_name)
    return player_initiative

def get_initiatives():
    "main function to get players and initiative"
    print('Welcome to Initiative Tracker!')
    loop_count = get_players()
    initiative_tracking = {}
    for i in range(loop_count):
        player_name = input('What is Player ' + str(i + 1) + '\'s name? ')
        player_initiative = get_initiative(player_name)
        if player_initiative in initiative_tracking:
            initiative_tracking[player_initiative].append(player_name)
        else:
            initiative_tracking[player_initiative] = [player_name]

    print('\nYour initiative order is: ')
    for key in sorted (initiative_tracking.keys(), reverse=True):
        print(str(key) + ': ' + ', '.join(initiative_tracking[key]))

if __name__ == '__main__':
    get_initiatives()

Hope this helps!

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks this helps a lot. Implemented some pieces and definitely going to keep in mind breaking up the functions in the future. \$\endgroup\$ – Alexander Jul 10 '16 at 3:15
1
\$\begingroup\$

Based on the different pieces of feedback I setup functions for the different task as well as implementing the iter(), "main", lambda and some other features that were useful to use and dig into after being suggested. In the future I plan to expand this to write initiatives to a JSON file with the dictionary timestamped and track a characters average initiative or something fun like that. Any other suggestions please let me know. I appreciate all the feedback and this has been an extremely education first script review. Thank you @zondo and @alec. I also put this out on GitHub: https://github.com/AlexHagerman/initiative_tracker

def validate_num_input(user_input):
    while True:
        try:
            num_input = int(input(user_input))
            break
        except ValueError:
            print("Please enter a valid number. ")
    return num_input

def get_initiative():
    print('Welcome to Initiative Tracker!')
    initiative_tracking = {}
    for player_name in iter(lambda: input("What is the players name? "), ''):
        player_initiative = validate_num_input('What is {} initiative? '.format(player_name))
        if player_initiative in initiative_tracking:
            initiative_tracking[player_initiative].append(player_name)
        else:
            initiative_tracking[player_initiative] = [player_name]
    return(initiative_tracking)

def print_initiative(init_list):
    print('\nYour initiative order is: ')
    for key in sorted (init_list, reverse=True):
        print('{}: {}'.format(key, ', '.join(init_list[key])))

if __name__ == '__main__':
    init_list = get_initiative()
    print_initiative(init_list)
\$\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.