4
\$\begingroup\$

Have been playing with the undocumented NHL API for some days. I'd love to see any input about this little script. About overall structure, style, performance enhancements, etc. Not sure how to properly use long links in Python, should I even follow the PEP in this case? The same problem in a couple of other lines, it exceeds 79 symbols limit. Not sure if I should use pluses in the 'printed_info' lines, it behaves the same way without it, but some say it's more explicit, so it's better.

import requests
import json
import os
import collections
import timeit
import datetime


def basic_bio():
    api_basic_bio = "http://www.nhl.com/stats/rest/skaters?isAggregate=false&reportType=basic&isGame=false&reportName=skaterpercentages&cayenneExp=gameTypeId=2%20and%20seasonId=20182019"

    json_data = requests.get(api_basic_bio).json()
    basic_full_bio_list = json_data["data"]
    return basic_full_bio_list


def lastnames():
    list_lastnames = []
    players_range = range(len(basic_bio()))
    json_bios = basic_bio()

    for number in players_range:
        list_lastnames.append((json_bios[number]["playerLastName"]).lower())

    return list_lastnames


def duplicate_last_names():
    last_names_count = collections.Counter(lastnames())
    dup_last_names = {k: v for k, v in last_names_count.items() if v > 1}
    return dup_last_names


def get_id_name_dict():
    players_id_dict = {}
    players_range = range(len(basic_bio()))
    dups = duplicate_last_names()
    skaters = basic_bio()

    for number in players_range:
        if (skaters[number]["playerLastName"]).lower() in dups:
            players_id_dict[skaters[number]["playerName"].lower()] = skaters[number]["playerId"]
        else:
            players_id_dict[skaters[number]["playerLastName"].lower()] = skaters[number]["playerId"]

    with open('nhl_skaters_id.json', 'w') as file:
         file.write(json.dumps(players_id_dict, indent=4, sort_keys=True))


def get_id(player_lastname):
    with open('nhl_skaters_id.json') as file:
      data = json.load(file)
    dups = duplicate_last_names()

    if player_lastname in dups:
        player_firstname = input("Enter a player's firstname: ").lower()
        return data[player_firstname + ' ' + player_lastname]
    else:
        return data[player_lastname]


def get_stats(id):
    api_base = 'https://statsapi.web.nhl.com/api/v1/people/'
    api_end = '?hydrate=stats(splits=statsSingleSeason)'
    url = ''.join([api_base, str(id), api_end])
    json_data = requests.get(url).json()
    stats = json_data["people"]
    return stats


def show_stats(data):
    path_stats = data[0]['stats'][0]['splits'][0]['stat']
    path_bio = data[0]

    printed_info = (f"Name: {path_bio['fullName']}\n\n" +
         f"Birth Date: {path_bio['birthDate']}\n" +
         f"Height: {path_bio['height']}\n" +
         f"Weight: {path_bio['weight']}\n\n" +
         f"Games: {path_stats['games']}\n" +
         f"Goals: {path_stats['goals']}\n" +
         f"Assists: {path_stats['assists']}\n" +
         f"Points: {path_stats['points']}\n" +
         f"PP Points: {path_stats['powerPlayPoints']}\n" +
         f"Plus-minus: {path_stats['plusMinus']}\n" +
         f"Shots: {path_stats['shots']}\n" +
         f"Hits: {path_stats['hits']}\n" +
         f"Blocks: {path_stats['blocked']}\n" +
         f"Penalty Minutes: {path_stats['pim']}\n" +
         f"TOI per Game: {path_stats['timeOnIcePerGame']}\n" +
         f"PP TOI per Game: {path_stats['powerPlayTimeOnIcePerGame']}\n" +
         f"SH TOI per Game: {path_stats['shortHandedTimeOnIcePerGame']}\n")

    print(printed_info)


def data_old(file_name):
    one_day_ago = datetime.datetime.now() - datetime.timedelta(days=1)
    filetime = datetime.datetime.fromtimestamp(os.path.getmtime(file_name))

    if filetime < one_day_ago:
        return True
    else:
        return False


player_lastname = input("Enter a player's lastname: ").lower()

if os.path.isfile('nhl_skaters_id.json') == False or data_old('nhl_skaters_id.json') == True:
    get_id_name_dict()

try:
    data = get_stats(get_id(player_lastname))
    print('-----------------------------')
    show_stats(data)
except Exception:
    print(f"{player_lastname.capitalize()} did't play any NHL games this season")
\$\endgroup\$
1
  • \$\begingroup\$ Just figured out that I could capitalize() both user inputs and do not use lower() and capitalize() calls in any other place of the program. \$\endgroup\$ Dec 6, 2018 at 13:13

1 Answer 1

5
\$\begingroup\$
  • One way to deal with long URLs that are long because of lots of parameters is to let requests deal with those parameters by passing a dictionary:

    URL = "http://www.nhl.com/stats/rest/{}"
    
    def basic_bio():
        params = {"isAggregate": "false",
                  "reportType": "basic",
                  "isGame": "false",
                  "reportName": "skaterpercentages",
                  "cayenneExp": "gameTypeId=2 and seasonId=20182019"}
        return requests.get(URL.format("skaters"), params=params).json()["data"]
    

    It even performs the urlencoding for you (by escaping the spaces in the last parameter, in this case).

  • Don't Repeat Yourself (DRY). In addition, always try to iterate over the elements of an iterable, instead of indices. This allows you to use iterable but not indexable things (like generators). For lastnames, you can use a list comprehension, though:

    def lastnames():
        return [player["playerLastName"].lower() for player in basic_bio()]
    
  • Python has multiline strings:

    PLAYER_STATS = """Name: {fullName}
    
    Birth Date: {birthDate}
    Height: {height}
    ...
    SH TOI per Game: {shortHandedTimeOnIcePerGame}
    
    """
    
    def show_stats(data):
        path_stats = data[0]['stats'][0]['splits'][0]['stat']
        path_bio = data[0]
        print(PLAYER_STATS.format(**path_bio, **path_stats))
    

    This uses the fact that in Python 3 you can keyword expand multiple mappings. It is OK if not all keys of the dictionary/ies are used.

  • Instead of comparing something directly with False or True, like in

    if os.path.isfile('nhl_skaters_id.json') == False or data_old('nhl_skaters_id.json') == True:
        # get info
    

    just do

    skaters_json = 'nhl_skaters_id.json'
    if not os.path.isfile(skaters_json) or data_old(skaters_json):
        # get info
    
  • Don't explicitly return True or False unless you really have to. Instead of

    if filetime < one_day_ago:
        return True
    else:
        return False
    

    Just do

    return filetime < one_day_ago
    
  • You should wrap your calling code in a if __name__ == "__main__": guard to allow importing from this script without executing those functions.
  • When catching exceptions, try to be as specific as possible. except Exception is already better than a bare except (because at least it does not prevent the user from exiting using Ctrl+C), but if you know the specific exception (or exceptions) you want to guard against, then use that knowledge. Here it is probably a IndexError or KeyError?
\$\endgroup\$
14
  • 1
    \$\begingroup\$ @Flynn84 Chaining calls like this is indeed a balancing act between simplicity (no need for intermediate variables which are only used once and for which you need to find sensible names) and readability (Chaining too many calls will obfuscate what the object represents). In general I tend to chain them if the intermediate objects are used nowhere, the length of the line stays within 80 chars and the chain is relatively easy to follow (like here). \$\endgroup\$
    – Graipher
    Dec 5, 2018 at 9:40
  • 1
    \$\begingroup\$ @Flynn84 And the part about explicitly returning True, I agree here the common practice seems to disagree with the Python Zen. However, remember that there is also "Simple is better than complex." and "Flat is better than nested." In Python duck-typing is often used ("If it quacks like a duck...it's probably a duck"). This means that many methods return only truthy or falsy objects, instead of True and False and that is fine. \$\endgroup\$
    – Graipher
    Dec 5, 2018 at 9:50
  • 1
    \$\begingroup\$ Yeah, I could've read docs more thoroughly, just read about the difference between params and data. \$\endgroup\$ Dec 5, 2018 at 13:03
  • 1
    \$\begingroup\$ @Flynn84: It knows it because both path_bio and path_stats are dictionaries that are keyword unpacked into the format call by the **. Therefore this boils down to PLAYER_STATS.format(fullName=path_bio['fullName'], ..., shortHandedTimeOnIcePerGame=path_stats['shortHandedTimeOnIcePerGame']. The only thing you need to be mindful of is that if a key exists in both dictionaries, this will raise a TypeError. \$\endgroup\$
    – Graipher
    Dec 5, 2018 at 15:58
  • 1
    \$\begingroup\$ @Flynn84: It takes so much more time because the duplicate_last_names function is called every loop iteration, instead of only once. \$\endgroup\$
    – Graipher
    Dec 6, 2018 at 14:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.