7
\$\begingroup\$

I'm working on a text-based football simulation game along the lines of Football Simulator. Below is a subset of my total code, specifically the functions used to create a new player. I also have functions (not shown) to create a new coach, create the teams, create the weekly schedules, etc. I'm hoping to be able to use the feedback I get here to improve those sections as well.

Before anyone suggests storing the data in a database, I started out that way, but ending up opting for dictionaries/lists instead for several reasons, so please try to look past that.

Anyway, here goes. The biggest thing I'm struggling with is having to pass a list (person_data) of all the parameters needed by create_new_player. I don't feel it's efficient to have to build up a list before calling the function, pass it, then have to deconstruct it inside the function. I know using global variables isn't recommended, so I'm not sure if there are any other options. I have to do similar things (albeit using a list of different parameters) for my other functions. I appreciate all feedback you may have.

EDIT: I made a mistake in my original post, I use player_id_index to keep track of how many players have been created, so that the next time I call create_new_player it starts where the previous one left off, even though it's not shown below.

# python3
import csv
from random import choice, randint, gauss

def create_names_first_data():
    '''
    create a list of all possible first names using text file as source data
    '''
    first_names = []
    filename_first = 'resources/names_first.txt'
    with open(filename_first, 'r') as file_to_open:
        for line in file_to_open:
            data = line.split()
            new_name = data[0]
            first_names.append(new_name)
    return first_names

def create_names_last_data():
    '''
    create a list of all possible last names using text file as source data
    '''
    last_names = []
    filename_last = 'resources/names_last.txt'
    with open(filename_last, 'r') as file_to_open:
        for line in file_to_open:
            data = line.split()
            new_name = data[0]
            last_names.append(new_name)
    return last_names

def create_states_data():
    '''
    create a list of all possible towns/state with their population 
    using text file as source data
    '''
    states = {}
    filename_states = 'resources/state_populations.csv'
    with open(filename_states, 'r') as file_to_open:
        reader = csv.DictReader(file_to_open)
        for i in reader:
            state_name = i['state']
            states[state_name] = {}
            states[state_name]['abbreviation'] = i['abbreviation']
            states[state_name]['population'] = i['population']
            states[state_name]['towns'] = {}

    towns = {}
    filename_towns = 'resources/city_populations_locations.csv'
    with open(filename_towns, 'r') as file_to_open:
        reader = csv.DictReader(file_to_open)
        for i in reader:
            town_name = i['city']
            state = i['state']
            towns[town_name] = {}
            towns[town_name]['population'] = i['population']
            towns[town_name]['latitude'] = i['latitude']
            towns[town_name]['longitude'] = i['longitude']

            for j in states:
                if state == states[j]['abbreviation']:
                    states[j]['towns'][town_name] = towns[town_name]
    return states

def get_home_town_state(states_dict):
    '''
    randomly pick a town/state
    '''
    states_list = [_ for _ in states_dict]
    state = choice(states_list)
    towns_dict = states_dict[state]['towns']
    towns_list = [_ for _ in towns_dict]
    town = choice(towns_list)
    return town, state

def generate_gauss_dist(mean, std_dev, minimum, maximum):
    '''
    utility function
    randomly generate a number based on a gauss distribution
    '''
    number = gauss(mean, std_dev)
    while (minimum <= number <= maximum) == False:
        number = gauss(mean, std_dev)
    number = int(round(number))
    return number

def create_new_player(param_list, seed=0, position=None):
    '''
    create a new player

    seed numbers are used to generate better/worse players as necessary

    if a position parameter is passed, that position will be used, otherwise
    it will be chosen based upon certain probabilities, i.e. - don't need
    many Ps/Ks

    player_interest is a dictionary of what the player cares about most when
    deciding which team to play for
    '''
    first_names_list = param_list[0]
    last_names_list = param_list[1]
    states_dict = param_list[2]
    positions_list = param_list[3]

    firstname = choice(first_names_list)
    lastname = choice(last_names_list)
    name = '{0} {1}'.format(firstname, lastname)

    town, state = get_home_town_state(states_dict)

    seeds = {
        0: {'mean': 50, 'min': 1,  'max': 100},
        1: {'mean': 92, 'min': 85, 'max': 99},
        2: {'mean': 82, 'min': 75, 'max': 89},
        3: {'mean': 69, 'min': 59, 'max': 79},
        4: {'mean': 54, 'min': 44, 'max': 64}
    }
    rating_seed = seeds[seed]
    mean = rating_seed['mean']
    std_dev = mean / 3.25
    minimum = rating_seed['min']
    maximum = rating_seed['max']
    rating = generate_gauss_dist(mean, std_dev, minimum, maximum)

    probability_list = [5, 6, 10, 5, 15, 15, 12, 13, 2, 2]
    position_probabilities = []
    for i in zip(positions_list, probability_list):
        position_probabilities.extend([i[0]] * i[1])

    if position is None:
        position = choice(position_probabilities)

    player_interests = {}
    player_interests['money'] = randint(1, 10)
    player_interests['play for a winner'] = randint(1, 10)
    player_interests['team facilities'] = randint(1, 10)
    player_interests['team location'] = randint(1, 10)
    player_interests['coach prestige'] = randint(1, 10)

    new_player = {
        'name': name,
        'position': position,
        'rating': rating,
        'player interests': player_interests,
        'home state': state,
        'home town': town
    }
    return new_player


if __name__ == '__main__':
    '''
    it isn't implemented using __name__==__main__ in the real thing,
    but you get the idea
    '''
    first_names_list = create_names_first_data()
    last_names_list = create_names_last_data()
    states_dict = create_states_data()
    positions_list = [
        'QB', 'RB', 'WR', 'TE', 'OL', 'DL', 'LB', 'DB', 'P', 'K'
    ]

    person_data = [
        first_names_list,
        last_names_list,
        states_dict,
        positions_list
    ]

    player_id_index = 0
    player_dict = {}

    for _ in range(5000):
        player_id_index += 1
        new_player = create_new_player(person_data)
        player_dict[player_id_index] = new_player
\$\endgroup\$
3
\$\begingroup\$

You don't need to define and increment the player_id_index variable outside of the for loop:

player_id_index = 0
player_dict = {}

for _ in range(5000):
    player_id_index += 1
    new_player = create_new_player(person_data)
    player_dict[player_id_index] = new_player

Just do:

for player_id_index in range(1, 5000):
    player_dict[player_id_index] = create_new_player(person_data)

Or reduce it all to this dict comprehension:

player_dict = {id_: create_new_player(id_) for id_ in range(1, 5000)}

Unpack the tuple in the head of this for loop to make it more readable:

for i in zip(positions_list, probability_list):
    position_probabilities.extend([i[0]] * i[1])

for position, probability in zip(positions_list, probability_list):
    position_probabilities.extend([position] * probability )

Python 3.6 offers you the new random.choices function:

positions_list = ['QB', 'RB', 'WR', 'TE', 'OL', 'DL', 'LB', 'DB', 'P', 'K']
probability_list = [5, 6, 10, 5, 15, 15, 12, 13, 2, 2]
position = random.choices(positions_list, probability_list, k=1)[0]

In create_states_data I think this would look a bit more readable (you could also use a dict comprehension here, but it'd look a bit dense):

for i in reader:
    states[i['state']] = {'abbreviation': i['abbreviation'],
                          'population': i['population'],
                          'towns': {}}

Regarding the global variables first_names_list, last_names_list etc., since you don't mutate them later, they are global constants. You can just access them in the create_character function and don't have to pass them. The convention is to use uppercase names for constants to signal to other programmers that they mustn't be changed, e.g. FIRST_NAMES_LIST.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I actually made a mistake copying over the code. I do need player_id_index (unless you could recommend a better solution) because it allows me to create additional players later on, and keep track of how many I've made so far. I know global variables are a no-no. Are global constants acceptable? \$\endgroup\$ – drewd423 Mar 31 '17 at 11:58
  • \$\begingroup\$ @drewd423 Global constants are bad. I'll have to revert your edit, answer invalidation is a big no-no around Code Review. \$\endgroup\$ – Mast Mar 31 '17 at 12:52
  • \$\begingroup\$ @Mast Sorry about that. What's the best way to show the corrected code? Just add another block? \$\endgroup\$ – drewd423 Mar 31 '17 at 13:34
  • \$\begingroup\$ @Mast please explain why global constants are bad. \$\endgroup\$ – skrx Apr 1 '17 at 1:25
  • 2
    \$\begingroup\$ @skrx Wow, that was a mistake on my part. Globals are bad, but constants are the one exception to the rule. Provided they're uppercase like you mentioned so they don't get changed accidentally. \$\endgroup\$ – Mast Apr 1 '17 at 8:47

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.