4
\$\begingroup\$

I built a minimal working model of a consumer of a RESTful API in Python 3.6. It only consumes one end point in this example. It is using the API from a video game (Destiny 2) to collect data about all the members of a clan (a clan is basically a team that likes to play together).

Some features

  • The core interface consists of a wrapper for requests (called request), and a class that summarizes the response (ResponseSummary).
  • URL-generating functions produce the URL for each end point.
  • Helper functions process the ResponseSummary.
  • Secret stuff (the API key) is stored as environment variables.

Questions

This is my first project interacting with an API, so any comments, or suggestions are welcome, as I may be making a stupid blunder here. Places of concern include, but are not limited to:

  • The ResponseSummary class seems a bit smelly to me. It includes all the data I want, but I'm not sure about defaulting all fields to None, the error handling, and the __repr__ may be too verbose.
  • Is my division of labor into core API code, URL-generators, and helper functions reasonable and Pythonic (is there a better way to do it that makes it more readable)?
  • Naming: are my variable names OK: in particular is the function request a bad name because it is too close to 'requests'?
  • Is the way I handled the headers argument for requests OK? I turned it into a constant (it is used in every API call).

The code

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import os
import requests

BASE_URL = 'https://bungie.net/Platform/GroupV2/'

"""
CORE CODE
For making requests, and summarizing the response
"""
def request(url, request_headers):
    response = requests.get(url, headers = request_headers)
    return ResponseSummary(response)


class ResponseSummary:
    """Important information about the response."""
    def __init__(self, response):
        self.status = response.status_code
        self.url = response.url
        self.data = None
        self.message = None
        self.error_code = None
        self.error_status = None
        self.exception = None
        if self.status == 200:
            result = response.json()
            self.message = result['Message']
            self.error_code = result['ErrorCode']
            self.error_status = result['ErrorStatus']
            if self.error_code == 1:
                try:
                    self.data = result['Response']
                except Exception as ex:
                    print("ResponseSummary: 200 status and error_code 1, but there was no result['Response']")
                    print("Exception: {0}.\nType: {1}".format(ex, ex.__class__.__name__))
                    self.exception = ex.__class__.__name__
            else:
                print('No data returned for url: {0}.\n {1} was the error code with status 200.'. \
                      format(self.url, self.error_code))
        else:
            print('Request failed for url: {0}.\n.Status: {0}'.format(self.url, self.status))

    def __repr__(self):
        """What will be displayed/printed for the class instance."""
        disp_header =       "<" + self.__class__.__name__ + " instance>\n"
        disp_data =         ".data: " + str(self.data) + "\n\n"
        disp_url =          ".url: " + str(self.url) + "\n"
        disp_message =      ".message: " + str(self.message) + "\n"
        disp_status =       ".status: " + str(self.status) + "\n"
        disp_error_code =   ".error_code: " + str(self.error_code) + "\n"
        disp_error_status = ".error_status: " + str(self.error_status) + "\n"
        disp_exception =    ".exception: " + str(self.exception)
        return disp_header + disp_data + disp_url + disp_message + \
               disp_status + disp_error_code + disp_error_status + disp_exception


"""
URL GENERATORS
"""
def get_members_of_group_url(group_id):
    """
    Pull all members of a clan.
    Documentation: https://bungie-net.github.io/multi/operation_get_GroupV2-GetMembersOfGroup.html
    """
    return BASE_URL + group_id + '/Members/?currentPage=1'


"""
HELPER FUNCTIONS
"""
def generate_clan_list(member_data):
    """
    Using GetMembersOfGroup end point, create list of member info for clan members.
        Each elt is a dict with username. id, join date. Filters out people not on psn.
    """
    member_data = member_data['results']
    clan_members_data = []
    for member in member_data:
        clan_member = {}
        clan_member['membership_type'] = member['destinyUserInfo']['membershipType']
        if clan_member['membership_type'] == 2:
            clan_member['name'] = member['destinyUserInfo']['displayName']
            clan_member['id'] = member['destinyUserInfo']['membershipId']
            clan_member['date_joined']  = member['joinDate']
            clan_members_data.append(clan_member)
    return clan_members_data



def print_clan_roster(clan_members_data):
    """Print name, membership type, id, and date joined."""
    if clan_members_data:
        name_list = [clanfolk['name'] for clanfolk in clan_members_data]
        col_width = max(len(word) for word in name_list)
        for clan_member in clan_members_data:
            memb_name = clan_member['name']
            length_name = len(memb_name)
            num_spaces = col_width - length_name
            memb_name_extended = memb_name + " "*num_spaces
            print("{0}\tMembership type: {1}\t Id: {2}\tJoined: {3}".format(memb_name_extended, \
                  clan_member['membership_type'], clan_member['id'], clan_member['date_joined']))
    else:
        print("print_clan_roster: roster is empty")


def get_environment_variable(var_name):
    """get environmental variable, or return exception"""
    try:
        return os.environ.get(var_name)
    except KeyError:
        error_msg = 'KeyError in get_environment_variable: {}.'.format(var_name)
        print(error_msg)


if __name__ == "__main__":
    """
    Extract and print list of all clan members 
    """
    #Set constants
    D2_KEY = get_environment_variable('D2_KEY')
    D2_HEADERS = {"X-API-Key": D2_KEY}
    CLAN_ID = '623172'

    #Make request to api for clan members, and print list to stdout
    get_members_url = get_members_of_group_url(CLAN_ID)
    get_members_summary = request(get_members_url, D2_HEADERS)
    member_data = get_members_summary.data
    clan_members_data = generate_clan_list(member_data)
    print_clan_roster(clan_members_data)
\$\endgroup\$
  • \$\begingroup\$ What version of Python are you using? \$\endgroup\$ – Ludisposed Dec 8 '17 at 15:12
  • \$\begingroup\$ @Ludisposed 3.6, I added this to doc. \$\endgroup\$ – cortical_iv Dec 8 '17 at 16:19
  • 2
    \$\begingroup\$ Do you have any reasons for not using f-strings? That’s one of the nicest features Python 3.6 came with. \$\endgroup\$ – Grajdeanu Alex. Dec 8 '17 at 16:46
  • \$\begingroup\$ @MrGrj, I have never used them, but will have to look at that. They seem like an improvement. \$\endgroup\$ – cortical_iv Dec 8 '17 at 19:30
7
\$\begingroup\$

Catching exceptions the way you do is not useful. For example if the D2_KEY environment variable isn't present, it's clear that the request will ultimately fail. Similarly if self.data = result['Response'] raises an exception then self.data is None which means that member_data = member_data['results'] fails with TypeError: 'NoneType' object is not subscriptable. Only catch the exception if you can actually do something useful about it. Similarly if the status or error code is unexpected, it's fine to display some useful data like the actual code value, but raise an exception right there to show the failure.

The ResponseSummary class could just be replaced by a method returning data. There are times when classes are helpful, but this is not one of them.

With these two things in mind, you can greatly simplify your code.

\$\endgroup\$
  • \$\begingroup\$ Yes I should burrow more into my exception handling for sure: this is basically a bare Except and at the very least I should catch some specific errors before punting. Also, while replacing ResponseSummary with data may be OK in this case, the other information in the ResponseSummary is actually useful in other cases I have left out to keep it simple. But if you were to say "Just use the response, it contains all that data", that might be valid. I did this to have a simpler container than the bare response returned by requests. However, I'm not sure if that is justified or not. \$\endgroup\$ – cortical_iv Dec 8 '17 at 19:42
1
\$\begingroup\$

As pointed out by Alex Hall, there are many places the above code could be improved:

  • Exception handling is nonexistent for requests, which tends to create lots of errors.
  • The ResponseSummary class is a monster: it doesn't raise any exceptions, but simply prints messages. In Python, exceptions are much better than error codes. Replace that class with a simpler response-handler that simply returns data, or raise an exception.
  • Instead of feeding requests.get the secret key every time, you should use a session and set the secret key to persist over the whole session.
  • Instead of displaying errors with print, use a logger. This gives you much more flexibility over what will be displayed, and under what conditions.

Below is a new and improved version that fixes all those problems. Tthere is room for improvement, mainly in the exception handling. That turned out to be the most challenging part of this code, and I am still learning.

import os
import requests
import json
import logging


BASE_URL = 'https://bungie.net/Platform/Destiny2/'
BASE_URL_GROUP = 'https://bungie.net/Platform/GroupV2/'

"""
Set up logger: for now just print everything to stdout.
"""
logging.basicConfig(level = logging.INFO,
                    format = '%(asctime)s - %(levelname)s - %(message)s',
                    datefmt =' %m/%d/%y %H:%M:%S')
logger = logging.getLogger(__name__)


"""
CORE CODE
Make requests, and extract data from response
"""
class BungieError(Exception):
    """Raise when ErrorCode from Bungie is not 1"""


def make_request(url, session):
    try:
        response = session.get(url)
        if not response.ok:
            response.raise_for_status()
    except requests.exceptions.RequestException as requestException:
        raise
    else:
        return response


def process_bungie_response(response):
    """Examines response from d2 if you got status_code 200, throws
    exception of type BungieException if bungie ErrorCode is not 1. For list of error
    codes, see:
        https://bungie-net.github.io/multi/schema_Exceptions-PlatformErrorCodes.html#schema_Exceptions-PlatformErrorCodes
    """
    response_url = response.url    #If you oops sent it something that can't be json'd
    try:
        response_json = response.json()
    except json.JSONDecodeError as jsonError:
        msg1 = f"JSONDecodeError in process_bungie_response().\n"
        msg2 = "Response does not contain json data.\n"
        msg3 = f"URL: {response_url}.\nError: '{jsonError}'"
        msg = msg1 + msg2 + msg3
        raise BungieError(msg) from jsonError

    try:
        data = response_json['Response']
    except KeyError as keyError:
        error_code = response_json['ErrorCode']
        error_status = response_json['ErrorStatus']
        error_message = response_json['Message']
        msg1 = f"KeyError in process_bungie_response.\nURL: {response_url}.\n"
        msg2 = f"Error code {error_code}: {error_status}.\nMessage: {error_message}.\n"
        msg = msg1 + msg2
        raise BungieError(msg) from keyError
    else:
        return data


def destiny2_api_handler(url, session):
    response = make_request(url, session)
    return process_bungie_response(response)


"""
URL GENERATORS
"""
def search_destiny_player_url(user_name):
    """Get user's info card:
        https://bungie-net.github.io/multi/operation_get_Destiny2-SearchDestinyPlayer.html
      Note for this example it's constrained to ps4 (platform = 2)
    """
    return BASE_URL + 'SearchDestinyPlayer/2/' + user_name + '/'


def get_members_of_group_url(group_id):
    """
    Pull all members of a clan.
        https://bungie-net.github.io/multi/operation_get_GroupV2-GetMembersOfGroup.html
    """
    return BASE_URL_GROUP + group_id + '/Members/?currentPage=1'


"""
HELPER FUNCTIONS
"""
def generate_clan_list(member_data):
    """
    Using GetMembersOfGroup end point, create list of member info for clan members.
        Each elt is a dict with username. id, join date. Filters out people not on psn.
    """
    member_data = member_data['results']
    clan_members_data = []
    for member in member_data:
        clan_member = {}
        clan_member['membership_type'] = member['destinyUserInfo']['membershipType']
        if clan_member['membership_type'] == 2:
            clan_member['name'] = member['destinyUserInfo']['displayName']
            clan_member['id'] = member['destinyUserInfo']['membershipId']
            clan_member['date_joined']  = member['joinDate']
            clan_members_data.append(clan_member)
    return clan_members_data


def print_clan_roster(clan_members_data):
    """Print name, membership type, id, and date joined."""
    if clan_members_data:
        name_list = [clanfolk['name'] for clanfolk in clan_members_data]
        col_width = max(len(word) for word in name_list)
        for clan_member in clan_members_data:
            memb_name = clan_member['name']
            length_name = len(memb_name)
            num_spaces = col_width - length_name
            memb_name_extended = memb_name + " "*num_spaces
            print("{0}\tMembership type: {1}\t Id: {2}\tJoined: {3}".format(memb_name_extended, \
                  clan_member['membership_type'], clan_member['id'], clan_member['date_joined']))
    else:
       print("print_clan_roster: roster is empty")


def get_environment_variable(var_name):
    """get environmental variable, or return exception"""
    try:
        return os.environ.get(var_name)
    except KeyError:
        error_msg = 'KeyError in get_environment_variable: {}.'.format(var_name)
        logger.error(error_msg)
        raise

if __name__ == "__main__":
    #Set constants
    D2_KEY = get_environment_variable('D2_KEY')
    D2_HEADERS = {"X-API-Key": D2_KEY}
    CLAN_ID = '623172'
    USER = 'cortical_iv'

    #Make the requests
    with requests.Session() as session:
        session.headers.update(D2_HEADERS)

        logging.info(f"Retrieving info about {USER}")
        search_player_url = search_destiny_player_url(USER)
        try:
            user_data = destiny2_api_handler(search_player_url, session)
        except Exception as e:
            logging.exception(f"Error getting user data for {USER}.\nException: {e}.")

        logging.info(f"Retreiving info about all members of clan {CLAN_ID}")
        get_members_url = get_members_of_group_url(CLAN_ID)
        try:
            members_data = destiny2_api_handler(get_members_url, session)
        except Exception as e:
            logging.exception(f"Error getting user data for {USER}.\nException: {e}.")
        else:
            clan_members_data = generate_clan_list(members_data)
            print_clan_roster(clan_members_data)
\$\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.