5
\$\begingroup\$

As a beginner to Python with some rookie experience in building software, I decided writing an API wrapper for Strawpoll would be a good exercise in learning the language and design patterns.

The intent was to finish writing the entire wrapper but I feel something is amiss, but am unable to identify why the code I've written doesn't feel as organized as other Python libraries I've seen.

I think I have done an OK job of separating and organizing the modules, but would love some comments on how this could be done better / in a more pythonic way.

Do let me know if you have any questions about my decision making / thought process I will try my best to answer those.

Below are the modules I would absolutely love some help with:

The base class:

strawpoll/base/strawpoll_base.py

"""
A python module to provide the base class for Strawpoll's JSON API:
https://strawpoll.me/api/v2/polls

The StrawpollAPIWriter class provides methods for:
* Calculations on fetched poll data
* Fetching poll data is left to the reader class to define
"""
import re


class StrawpollAPIBase(object):
    """
    TODO: Document this class
    """
    API_KEYWORDS = frozenset(
        [u'title', u'options', u'votes',
         u'multi', u'permissive', u'id', u'captcha'])

    API_ENDPOINT = 'https://strawpoll.me/api/v2/polls/'

    URL_PATTERN = re.compile('^https?://strawpoll.me/(?P<id>[1-9][0-9]*)/?r?')

    USER_AGENT = 'Strawpoll API Reader'

    API_POST_HEADERS = {
        'Content-Type': 'application/json',
        'X-Requested-With': 'StrawpollAPIWriter, github=http://git.io/vsV1E'
    }

    def __init__(self, data={}):
        self.id = None
        self.title = None
        self.options = None
        self.votes = None
        self.multi = False
        self.permissive = False
        self.captcha = False

        for key in data.keys():
            try:
                setattr(self, key, data[key])
            except AttributeError:
                continue

    def __eq__(self, other):
        return self.__dict__ == other.__dict__

    # Begin instance methods
    def total_votes(self):
        """ Returns the sum of votes cast for all option in a strawpoll """
        return sum(self.votes)

    def normalize(self):
        """ Returns Normalized votes on a 0.0 - 1.0 scale """
        total = self.total_votes()
        return [vote / total for vote in self.votes]

    def votes_for(self, option):
        """
        Returns the number of votes an option recieved
        Return None if no such option exists
        """
        try:
            return self.votes[self.options.index(option)]
        except ValueError:
            return None

    def normalized_votes_for(self, option):
        """
        Returns the fraction of votes an option recieved
        Return None if no such option exists
        """
        return self.normalize()[self.options.index(option)]

    def winner(self):
        """ Returns the option that got the most votes """
        most_popular_index = self.votes.index(max(self.votes))
        return self.options[most_popular_index]

    def loser(self):
        """ Returns the option that got the least votes """
        least_popular_index = self.votes.index(min(self.votes))
        return self.options[least_popular_index]

    def to_clean_dict(self):
        """
        Cleans up self.__dict__ so that it is accepted as json by strawpoll API
        """
        cdict = self.__dict__
        for key in cdict.keys():
            if cdict[key] == None:
                del cdict[key]
        return cdict

This is the API reader class that derives from strawpoll.base:

strawpoll/strawpoll_api_reader.py

"""
A python module to provide methods to read data from existing polls using
Strawpoll's JSON API (https://strawpoll.me/api/v2/polls).
The StrawpollAPIReader class provides methods for:
* Capturing all poll data in an instance
* Performing basic options such as normalizing votes for each option
* Finding the winner / loser
"""

from __future__ import division
from base.strawpoll_base import StrawpollAPIBase

import requests
import json


class StrawpollAPIReader(StrawpollAPIBase):

    def __init__(self, data={}):
        """ Construct self using a dictionary of data """
        super(StrawpollAPIReader, self).__init__()
        for key in data.keys():
            # This actually worked.
            # hasattr -> setattr died with AttributeErrors
            try:
                setattr(self, key, data[key])
            except AttributeError:
                # Log this?
                continue

    @classmethod
    def from_json(cls, json_string):
        """
        Constructs a poll instance from a JSON string
        returned by strawpoll.me API
        """
        api_response = json.loads(json_string)
        response_keys = set(api_response.keys())
        if response_keys.issubset(cls.API_KEYWORDS):
            return cls(data=api_response)

    @classmethod
    def from_apiv2(cls, id):
        """ Constructs a poll instance using a strawpoll id """
        response = requests.get(cls.API_ENDPOINT + str(id))
        return cls.from_json(response.text)

    @classmethod
    def from_url(cls, url):
        """
        Constructs a poll instance using a strawpoll url, that matches:
        ^https?://strawpoll.me/[1-9][0-9]*/?r?
        Issues: Still matches 'http://strawpoll.me/1r', but ignores the r at
        the very end
        """
        matches = cls.URL_PATTERN.match(url)
        if matches is not None:
            # Note: we are actually passing a str and not an int
            return cls.from_apiv2(matches.group('id'))

Please let me know what you think is a better way to organize these two, if what I have done isn't good enough.

\$\endgroup\$
5
  • 3
    \$\begingroup\$ Welcome to Code Review! Unfortunately your question is off-topic as of now, as the code to be reviewed must be present in the question. Please add the code you want reviewed in your question. Thanks! \$\endgroup\$ Commented Sep 24, 2015 at 11:24
  • \$\begingroup\$ @SuperBiasedMan Thanks for pointing that out. I was unaware of this rule. Could you suggest a better way for me to address this? I will try to make the changes at the earliest as I am work right now. \$\endgroup\$
    – 0xC0FFEE
    Commented Sep 24, 2015 at 11:29
  • 1
    \$\begingroup\$ Depending on how much code you have, either edit it all into the question or select the parts that you most want feedback on put them there. In both cases it's helpful to keep the github link but you should definitely move some code into the question. \$\endgroup\$ Commented Sep 24, 2015 at 11:31
  • 2
    \$\begingroup\$ Sure, the library is actually quite small. I'll edit in the required modules once I reach home \$\endgroup\$
    – 0xC0FFEE
    Commented Sep 24, 2015 at 11:33
  • \$\begingroup\$ @SuperBiasedMan let me know if the edit suffices or if it should be changed. I'm unfamiliar with codereview.stackexchange so please excuse any errors I may have unknowingly made. \$\endgroup\$
    – 0xC0FFEE
    Commented Sep 24, 2015 at 19:55

1 Answer 1

1
\$\begingroup\$

Documentation

I think my first issue with this is this:

""
TODO: Document this class
"""

Angels weep when you write code without documenting it first. In general, you should know what it's supposed to do before you write it. Implementation details are irrelevant, and shouldn't go in your documentation anyway (they belong in comments).

ABC

That aside, I don't think I see the point of the base class. Unless there are other StrawpollAPIs you want to implement that should subclass this.

__init__

Additionally, I don't like how you write __init__. Why are you doing this?

for key in data.keys():
    try:
        setattr(self, key, data[key])
    except AttributeError:
        continue

You shouldn't be ignoring errors like that - at the very least log the error (like you comment about in the subclass). Beyond that though, it seems to me that there should be a finite number of known keys in the API - accept them in the constructor as keyword arguments with sensible default values. After all, to quote the Zen of Python,

Explicit is better than implicit.

I'd say something like

def __init__(self, id_=None, title=None, options=None ....):
    self.id = id_
    ...

is much cleaner, and doesn't require a major difference in how you call it:

cls(**api_response)

instead of

cls(data=api_response)

Then you loudly get an error if it has weird, invalid data. If you really still want to handle it, then use

def __init__(self, id_=None, ..., **kwargs):

and then handle extra keyword arguments appropriately.

One possible issue is with the name id_ - unfortunately id is a builtin function in Python, and using it as a parameter will override it locally. Also unfortunately, in all likelihood the API won't have the name id_. You can always access id from the __builtins__ module, or set a class member like

class StrawpollAPIBase(object):

    id_function = id

    def __init__(self, id=None, ...):

and then just use id_function if you need to use id inside of __init__.

Properties

I'd say things like normalize could be properties.

@property
def normalize(self):
    return [vote / self.total_votes() for vote in self.votes]

so then you can access it as instance.normalize, but this is a minor detail.

Magic methods

If you implement __eq__ make sure you implement __ne__ as well. It's easy

def __ne__(self, other):
    return not self == other

Whitespace

Add an empty line after your docstrings. Just a style concern, and pretty minor.

Naming

I don't like the name from_apiv2. It isn't very descriptive to me. Instead, maybe something like from_strawpoll_id or just from_id.

Also, and this is personal preference, I like to use the form Api instead of API while using acronyms in names, especially if they're followed by another CamelCased word.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the response, that was very informative and detailed! \$\endgroup\$
    – 0xC0FFEE
    Commented Sep 26, 2015 at 17:00

Your Answer

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

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