current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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
share|improve this question
    
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. –  itsbruce Jan 9 '14 at 13:40
    
Updated question, is this any cleaner? –  Tom celic Jan 9 '14 at 13:52
    
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. –  itsbruce Jan 9 '14 at 14:00
    
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? –  Tom celic Jan 9 '14 at 17:32
    
+1 for linking to Python is not Java. :) –  Aaron Hall Jan 10 '14 at 23:28

2 Answers 2

up vote 5 down vote accepted

  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.

share|improve this answer
    
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) –  Tom celic Jan 9 '14 at 13:16
    
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? –  Tom celic Jan 9 '14 at 13:41
    
@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. –  itsbruce Jan 9 '14 at 14:15
up vote 1 down vote

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()
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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