Becoming more pythonic

Question

I started porting an API wrapper from java to python for practice. I am looking for ways to improve the readability/maintainability this code.

I have done some reading about "pythonic" style and I am hoping someone could provide some feedback as to how one goes about making their code more pythonic.

All source can be found here: https://github.com/mccannm11/helpscout-api-python

The models module is what contains the custom types that the parser creates.

import requests
import json
import base64
import models

class ApiClient:
    BASE_URL = "https://api.helpscout.net/v1/"
    apiKey = ""

    def getMailbox(self, mailbox_id, fields=None):
        url = "mailboxes/" + str(mailbox_id) + ".json"
        if fields != None:      
            url = self.setFields(url, fields)
        return self.getItem(url, "Mailbox", 200)

    def getMailboxes(self, fields=None):
        url = "mailboxes.json"
        if fields != None:
            url = self.setFields(url, fields)
        return self.getPage(url,"Mailbox", fields)

    def getFolders(self, mailbox_id, fields=None):
        url = "mailboxes/" + str(mailbox_id) + "/folders.json"
        if fields != None:    
            url = self.setFields(url, fields)
        return self.getPage(url, "Folder", 200)

    def getConverstationsForFolders(self, mailbox_id, folder_id, fields=None):
        url = "mailboxes/" + str(mailbox_id) + "/folder/" + str(folder_id) + "conversations.json"
        if fields != None:     
            url = self.setFields(url, fields)
        return self.getPage(url, "Conversation", 200)

    def getConversationsForMailbox(self, mailbox_id, fields=None):
        url = "mailbox/" + str(mailbox_id) + "/conversations.json"
        if fields != None :
            url = self.setFields(url, fields)
        return self.getPage(url)

    def getConversationForCustomerByMailbox(self, mailbox_id, customer_id, fields=None):
        url = "mailboxes/" + str(mailbox_id) + "/customers/" + str(customer_id) + "/conversations.json"
        if fields != None:
            url = self.setFields(url, fields)
        return self.getPage(url, "Conversation", 200)

    def getConversation(self, conversation_id, fields=None):
        url = "conversations/" + str(conversation_id) + ".json"
        if fields != None:
            url = self.setFields(url, fields)
        return self.getItem(url, "Conversation", 200)

    def getAttachmentData(self, attachment_id):
        url = "attachments/" + str(attachment_id) + "/data.json" 
        json_string = self.callServer(url, 200)
        json_obj = json.loads(json_string)
        item = json_obj["item"]
        return item["data"]

    def getCustomers(self, fields=None):
        url = "customers.json"
        if fields != None:
            url = self.setFields(url, fields)
        return self.getPage(url, "Customer", 200)

    def getCustomer(self, customer_id, fields=None):
        url = "customers/" + str(customer_id) + ".json"
        if fields != None:
            url = self.setFields(url, fields)
        return self.getPage(url, "Customer", 200)

    def getUser(self, user_id, fields=None):
        url = "users/" + str(user_id) + ".json"
        if fields != None:
            url = self.setFields(url, fields)
        return self.getItem(url, "User", 200)

    def getUsers(self, fields=None):
        url = "users.json"
        if fields != None:
            url = self.setFields(url, fields)
        return self.getPage(url, "User", 200)

    def getUsersForMailbox(self, mailbox_id, fields=None):
        url = "mailboxes/" + str(mailbox_id) + "users.json"
        if fields != None:
            url = self.setFields(url, fields)
        return getPage(url, "User", 200)

    def callServer(self, url, expected_code):
        auth =  "Basic " + self.getEncoded() 
        headers= {'Content-Type': 'application-json'
                  , 'Accept' : 'application-json'
                  , 'Authorization' : str(auth)
                  , 'Accept-Encoding' : 'gzip, deflate'
                  }
        r = requests.get(self.BASE_URL + url, headers=headers)
        self.checkStatusCode(r.status_code, expected_code)
        return r.text

    def getItem(self, url, clazz, expected_code):
        string_json = self.callServer( url, expected_code )
        return Parser.parse(json.loads(string_json)["item"], clazz)

    def getPage(self, url, clazz, expected_code):
        string_json = self.callServer(url, expected_code)
        json_obj = json.loads(string_json)
        p = Page()

        for i in json_obj:
            setattr(p, i, json_obj[i])
        return p

    def getEncoded(self):
        raw = str(self.apiKey) + ":x"
        return base64.b64encode(raw)

    def getDecoded(val):
        return base64.b64decode(val)

    def setFields(self, url, fields):
        final_str = url + "?fields="
        if (fields != None and len(fields) > 0 ):
            sep = ""
            for i in fields:
                final_str += sep + fields[i]
                sep = ","

        return final_str

    def checkStatusCode(self, code, expected):
        if code == expected:
            return
        """ @todo gotta be a better way to do this """
        if (code == 400):
            raise Exception("The request was not formatted correctly")
        elif(code == 401):
            raise Exception("Invalid Api Key")
        elif(code == 402):
            raise Exception("API Key Suspended")
        elif (code == 403):
            raise Exception("Access Denied")
        elif (code == 404):
            raise Exception("Resource Not Found")
        elif (code == 405):
            raise Exception("Invalid method Type")
        elif(code == 429):
            raise Exception("Throttle Limit Reached. Too Many requests")
        elif(code == 500):
            raise Exception("Application Error or server error")
        elif(code == 503):
            raise Exception("Service Temporarily Unavailable")
        else:
            raise Exception("API Key Suspended")


class Page:
    def __init__(self):
        self.page = None
        self.pages = None
        self.count = None
        self.items = None

class ApiException(Exception):
    def __init__(self, message):
        Exception.__init__(self,message)


class Parser:
    @staticmethod
    def parse(json, clazz):
        c = getattr(globals()["models"], clazz)()
        for i in json:
            setattr(c, i, json[i])
        return c 

Answer 1

class ApiClient:

If using Python 2.x, consider inheriting from object. It makes a couple of advanced features work.

    BASE_URL = "https://api.helpscout.net/v1/"
    apiKey = ""

Class constants, like this apiKey, should be in ALL_CAPS

def setFields(self, url, fields):

The Python style guide recommends lowercase_with_underscores for method names. The name is also misleading, but it suggests that you are setting a fields property on the object.

    final_str = url + "?fields="
    if (fields != None and len(fields) > 0 ):

You don't need those parens. Its best to check against None using is not None. As @Ichau suggested, you can actually just check if fields:

        sep = ""
        for i in fields:

Use for key, value in fields.items() to avoid having to relookup each key

            final_str += sep + fields[i]
            sep = ","

Adding strings together is not a good idea because its not very efficient. Better to put all the pieces in a list and join it. The name sep is also confusing because I'd think it stands for seperator, but that's not how you are using it.

    return final_str

def getMailbox(self, mailbox_id, fields=None):

There is very little point to prefixes like get. More pythonic would be to call this method mailbox.

    url = "mailboxes/" + str(mailbox_id) + ".json"
    if fields != None:      

Firstly, setFields already handles None correctly. There's no point in checking for it here. Secondly, you do these two lines in pretty much every method. It'd be better to pass the fields parameter to getItem/getPage/etc and handle it consistently in all cases.

        url = self.setFields(url, fields)
    return self.getItem(url, "Mailbox", 200)


def getDecoded(val):
    return base64.b64decode(val)

You seem to be missing a self parameter

def checkStatusCode(self, code, expected): if code == expected: return

I don't like the happy case bailing out like this. I'd use pass/else rather then a return here.

    """ @todo gotta be a better way to do this """
    if (code == 400):
        raise Exception("The request was not formatted correctly")
    elif(code == 401):
        raise Exception("Invalid Api Key")
    elif(code == 402):
        raise Exception("API Key Suspended")
    elif (code == 403):
        raise Exception("Access Denied")
    elif (code == 404):
        raise Exception("Resource Not Found")
    elif (code == 405):
        raise Exception("Invalid method Type")
    elif(code == 429):
        raise Exception("Throttle Limit Reached. Too Many requests")
    elif(code == 500):
        raise Exception("Application Error or server error")
    elif(code == 503):
        raise Exception("Service Temporarily Unavailable")

Rather then all that, have a global dict mapping numbers to strings and use that. Also, why Exception and not ApiException?

    else:
        raise Exception("API Key Suspended")

class Parser:

This doesn't really fit the definition of a parser.

    @staticmethod
    def parse(json, clazz):

Why make it a staticmethod rather than just a global function?

        c = getattr(globals()["models"], clazz)()

You can just refer to models here, no need to mess with globals. Furthermore instead of passing strings around, you can just the class directly and skip this lookup altogether.

        for i in json:
            setattr(c, i, json[i])

I don't like doing this. The problem is that you don't know what is in that json and so any attributes whatsoever can be set. I'd maintain a list of the attributes I expect and check those against the ones present.

        return c 

Answer 2

Depends on your definition of "pythonic", if you haven't already check out already PEP 20, "The Zen of Python" gives some good pragmatic guidelines and PEP 8 gives some style guidelines.

Personally, pythonic or not, I like things to be easy to read and concise, so here are a couple suggestions

def getFolders(self, mailbox_id, fields=None):
    # if there's no mailbox_id shouldn't this throw an Exception?
    url = "mailboxes/%s/folders.json" % mailbox_id
    if fields:
        url = self.setFields(url, fields)
    return self.getPage(url, "Folder", 200)

Since this doesn't depend on anything in the class and looks like a utility method, you could just move it outside of the class completely and omit self and you might want to check if the url is None

def setFields(url, fields):
    # checks None and length
    if not fields:
        return url
    # if speed is important, you can concatenate instead
    # return url + "?fields=" + ",".join(fields);
    return "%s?fields=%s" % (url, ",".join(fields))

Here's an alternative to the if/else statements to simulate case/switch:

def checkStatus(self, code, expected):
    if code == expected:
        return
    def error_codes(code):
        messages = { 
            400 : "The request was not formatted correctly",
            401 : "Invalid API Key" }
        default_message = "API Key Suspended"
        return messages.get(code, default_message)
    raise Exception(error_codes(code))