3
\$\begingroup\$

I got the go ahead to make an API wrapper for an undocumented web API. I'm hoping to get some feedback on my code and design choice.

I've been going back and forth and haven't been able to really feel satisfied with any of my design choices.

There's a lot of placeholders, but I think the base of what I am going for should make sense. At this point it would be a matter of adding the endpoints and do the error catching if I were to continue with it as-is.

Tips, alternative design choices, really anything is welcome, and I appreciate it all.

from uuid import uuid4
from time import strftime, gmtime

from hashlib import md5

import requests


class Wrapper:

    endpoints = ['sign_in', 'sign_up']

    def __init__(self):
        # Details
        self.prv_key = ''  # private_key
        self.pub_key = ''  # token
        self.member_id = 0
        self.device_id = uuid4()

        # 'X-Device' header
        self.device_header = \
            '{0},{1},{2}:{2}'.format('android', self.device_id, '845')

    @property
    def auth_token(self):
        auth_token = ','.join((

            # MD5 of auth and time stamp
            md5(self.prv_key + self.pub_key).hexdigest(),
            md5(strftime('%Y-%m-%dT%H:%M,mobile', gmtime())).hexdigest(),

            # Member ID can't be int(0)
            str(self.member_id or '')

        ))

        return auth_token

    def request(self, url, method, **kwargs):
        # Default headers
        kwargs['headers'] = {
            'X-AuthToken':     self.auth_token,
            'X-Device':        self.device_header,
            'Accept':          'application/json',
            'Accept-Language': 'en-US',
            'User-Agent':      'okhttp/3.6.0'
        }
        # Proxy settings
        kwargs['proxies'] = {
            'https':           '192.168.178.27:8085',  # TODO: DEBUG
            'http':            '192.168.178.27:8085'   # TODO: DEBUG
        }
        # SSL verification
        kwargs['verify'] = False  # TODO: DEBUG

        r = requests.request(method, url, **kwargs)

        # TODO: CATCH ERRORS
        # TODO: CORRECT RESPONSE
        response = r.content

        return response

    def sign_in(self):
        return self.request('http://httpbin.org/get', 'GET')  # TODO: DEBUG

    def sign_up(self):
        return ''


if __name__ == '__main__':

    test = Wrapper()
    print test.sign_in()
\$\endgroup\$

2 Answers 2

Reset to default
1
\$\begingroup\$

Documentation

The PEP 8 style guide recommends adding docstrings for classes and functions. For example:

class Wrapper:
    """ Wrapper for web API """

Comments

To reduce clutter, remove all the "TODO" comments, such as:

# TODO: CATCH ERRORS
# TODO: CORRECT RESPONSE

If there are known limitations to the code, they can be documented in the docstrings.

Naming

The class name Wrapper is too generic. It should be changed to be more specific to the application.

This variable name is too cryptic:

self.prv_key = ''  # private_key

It should use the whole word, thereby eliminating the need for the comment:

self.private_key = ''

Simpler

These 2 lines:

response = r.content

return response

can be combined into 1 line, eliminating the intermediate variable:

return r.content

Layout

I admire the desire to keep lines short:

self.device_header = \
    '{0},{1},{2}:{2}'.format('android', self.device_id, '845')

but it can all be on one line:

self.device_header = '{0},{1},{2}:{2}'.format('android', self.device_id, '845')

Portability

I realize this question was posted many years ago when Python version 2.x was prevalent, but now that it is deprecated, consider porting to 3.x. Change:

print test.sign_in()

to:

print(test.sign_in())
\$\endgroup\$
1
\$\begingroup\$

This is clearly local configuration:

        # Proxy settings
        kwargs['proxies'] = {
            'https':           '192.168.178.27:8085',  # TODO: DEBUG
            'http':            '192.168.178.27:8085'   # TODO: DEBUG
        }

Embedding this into your code means that it's only useful on your local network where those addresses refer to the hosts you expect. On my network, there's no listener at those addresses.

The alternative I would encourage is to separate out that kind of local configuration (standard practice on Unix-like platforms is to use https_proxy and http_proxy environment variables) rather than hard-coding it. That makes your code useful to others.

It turns out that doing this is very easy, because if you don't specify any proxies, then the requests library does The Right Thing:

When the proxies configuration is not overridden per request as shown above, Requests relies on the proxy configuration defined by standard environment variables http_proxy, https_proxy, no_proxy, and all_proxy.

Here we have some comments that suggest the code is not completely finished:

        # TODO: CATCH ERRORS
        # TODO: CORRECT RESPONSE

It won't surprise you that my recommendation here is to do the TODOs and remove the comments.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.