4
\$\begingroup\$

I am pretty new to Python and just wanted to make sure I am going in the right direction with my coding style.

After reading this, I'm not so sure.

The following is a convenience class that has operations used a lot in my main code.

import requests

try: import simplejson as json
except ImportError: import json

"""
Basic operations involving users
"""

#The URL of the API
api_root = "http://api.stuff.com/v1/core";

class UserOperations(object):
    def __init__(self, headers):
        self.headers = headers

    def move_user(self,source_account,destination_account, user_id):

        user_id = str(user_id)

        #Get user
        user = self.get_user(source_account, user_id)

        if user is not None:

            #Remove user from source account
            self.remove_user(source_account, user_id)

            #Add user to destination account
            self.add_user(destination_account, user)

    def get_user(self, account_id, user_id):

        #Get user
        user = requests.get(api_root + "/accounts/" + account_id + "/users/" + user_id, headers=self.headers)

        #Throw exception if non-200 response
        user.raise_for_status()

        print "\nGet User " + user_id + ": " + user.text

        #Check user exists
        if user.json()['Data'] is None:
            return None

        #Return user
        return user.json()['Data']

    def remove_user(self,account_id, user_id):

        #Delete a user
        result = requests.delete(api_root + "/accounts/" + account_id + "/users/" + user_id, headers=self.headers)                
        #Throw exception if non-200 response
        result.raise_for_status()
        #Result of delete operation
        print "\nDelete Result " + result.text

    def add_user(self,account_id, user):

        #Add user to new account
        result = requests.post(api_root + '/accounts/' + account_id + '/users', data=json.dumps(user), headers=self.headers)
        #Throw exception if non-200 response
        result.raise_for_status()
        #Result of add operation
        print "\nAdd User: " + result.text

Does this seem on the right track or are there any Python mantra's I'm missing out on.

Edit

Function to replace below code duplication

variable = requests.get(api_root + "/accounts/"....)
variable.raise_for_status()

Function:

def request(self, request, account_id, user):

    if request is 'get':
        #Get user
        result = requests.get(api_root + "/accounts/" + account_id + "/users/" + user_id, headers=self.headers)
    elif request is 'post':
        #Add user to new account
        result = requests.post(api_root + '/accounts/' + account_id + '/users', data=json.dumps(user), headers=self.headers)
    elif request is 'delete':
        #Delete user from account
        result = requests.delete(api_root + "/accounts/" + account_id + "/users/" + user_id, headers=self.headers)

    #Throw exception if non-200 response
    result.raise_for_status()
    return result

Edit Function attempt 2:

def request(self, request, account_id, user):

    function_dict = {}
    function_dict['get'] = requests.get(api_root + "/accounts/" + account_id + "/users/" + user, headers=self.headers)
    function_dict['post'] = requests.post(api_root + '/accounts/' + account_id + '/users', data=json.dumps(user), headers=self.headers)
    function_dict['delete'] = requests.delete(api_root + "/accounts/" + account_id + "/users/" + user, headers=self.headers)

    result = function_dict.get(request)

    #Throw exception if non-200 response
    result.raise_for_status()
    return result
\$\endgroup\$
  • \$\begingroup\$ Ugh, if..elif..elif That's a bad smell. Use a dictionary to map the request verb to the appropriate function. Raise an exception if there is no key in the dictionary to match the request method. \$\endgroup\$ – itsbruce Jan 9 '14 at 13:40
  • \$\begingroup\$ Updated question, is this any cleaner? \$\endgroup\$ – TomSelleck Jan 9 '14 at 13:52
  • \$\begingroup\$ Some. But now look at those three lines where you define the functions to go in the dictionary; see how much duplication there is. You shouldn't be retyping almost the same thing again and again. The OO solution (you being an alleged Java geek) would be to create a request object that knows how to construct a basic request and uri, and specialisations thereof which can fill in the blanks for a particular verb. Those objects could go into a dictionary and their methods called as necessary. I'm sure there are more idiomatic alternatives that also avoid the duplication. \$\endgroup\$ – itsbruce Jan 9 '14 at 14:00
  • \$\begingroup\$ Right I tried a couple of things but keep ending up with my second example, any chance you could shed some light on this mysterious enigma? \$\endgroup\$ – TomSelleck Jan 9 '14 at 17:32
  • \$\begingroup\$ +1 for linking to Python is not Java. :) \$\endgroup\$ – Aaron Hall Jan 10 '14 at 23:28
6
\$\begingroup\$
  1. Unlike in Java, not everything in Python needs to be a class. In Python we tend to reserve classes for representing things with individual state and common behaviour. If you just want to group some functions together, you can put them in a module.

  2. You have no documentation. What do these functions do and how am I supposed to call them? In Python you should give every public class, function and method a docstring.

  3. Why do you try importing simplejson? This is just the externally maintained version of the built-in json module. You don't seem to use any simplejson-specific feature, so what's the point?

  4. Your code is not portable to Python 3, but this would be easy to fix by putting parentheses around the arguments to print.

  5. This code:

    #Check user exists
    if user.json()['Data'] is None:
        return None
    
    #Return user
    return user.json()['Data']
    

    can be simplified to:

    return user.json()['Data']
    
  6. You have code like this:

    VARIABLE = requests.METHOD(api_root + '/accounts/' + account_id + STUFF, headers=self.headers)
    VARIABLE.raise_for_status()
    

    in three places. You should make this a function.

  7. It's not clear what the point of this line is:

    user_id = str(user_id)
    

    All you ever do with user_id is add it to strings, where it will be converted to a string in any case.

  8. I prefer to use str.format to build up strings, rather than +: the former tends to result in clearer code. For example:

    api_root + "/accounts/" + account_id + "/users/" + user_id
    

    would become:

    '{}/accounts/{}/users/{}'.format(api_root, account_id, user_id)
    

Comment on updated question

You have failed to avoid the duplicated code: you have just moved it around.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for the advice! Just a couple of questions, 1) To turn this piece of code into a module, would I just remove the class definition? 2)Each of those functions require a variable called headers, would it be better to pass them in when calling the function? 3)How does the function look for removing the duplicate code? (Updated question) \$\endgroup\$ – TomSelleck Jan 9 '14 at 13:16
  • \$\begingroup\$ I'm afraid I can't spot how to remove the code duplication, how can I make a generic function and differentiate between different types of requests? \$\endgroup\$ – TomSelleck Jan 9 '14 at 13:41
  • \$\begingroup\$ @Tomcelic You really need to go back and re-read that page you linked to. I just read it and I can see one issue (if..elif) to which the author gives the same solution I did - so since you read that page before coming here, why didn't you take his advice. There is more advice on his page which would also help. \$\endgroup\$ – itsbruce Jan 9 '14 at 14:15
1
\$\begingroup\$

Agreeing with Gareth, here are my additions:

  1. Don't use compound statements; better formatting (as recommended by PEP 8):

    try:
        import simplejson as json
    except ImportError:
        import json
    
  2. I don't add strings either, when I can avoid it; my preferred solution would be (very suitable for i18n, by the way):

    '%(api_root)s/accounts/%(account_id)s/users/%(user_id)s' % locals()
    
|improve this answer|||||
\$\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.