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 2 down vote favorite

I've written this snippet to extract a review request status from review-board in python. It works well but i'm very new to python and i would like to know if i could write this any better.
Should i put conn.close in it's own try statement ?

This script is supposed to run as part of a post-commit hook in mercurial. If the review-board server is not responding i still want to be able to commit without any issues. If the commit is non existent then an empty string is ok.

import httplib
import json

def getReviewStatus(reviewboardUrl, id):
    try:
        conn = httplib.HTTPConnection(reviewboardUrl, 80)
        headers = {
            'Accept': 'application/json',
            'Content-Type': 'application/json; charset=UTF-8'
        }
        conn.request("GET", "/api/review-requests/%s/" % id, headers=headers)
        return json.loads(conn.getresponse().read())['review_request']['status']
    except:
        return ""
    finally:
        conn.close()
share|improve this question
1  
is this a standalone script? if so I'd let it blow on errors, don't catch any exceptions (specially don't silently catch all exceptions) –  tokland May 30 '13 at 9:45
    
i amended my question –  Willem D'haeseleer May 30 '13 at 9:52
add comment

1 Answer

up vote 1 down vote accepted

Looks pretty good to me. Regarding conn.close(), definitely don't put this in its own try statement, because if there's a problem with the earlier code, the connection might not get closed. I know you're not using Python 3, but ss an alternative, you might consider using the with statement and let Python handle the closing for you. See http://docs.python.org/2/tutorial/inputoutput.html#methods-of-file-objects (end of section) and http://docs.python.org/2/library/contextlib.html#contextlib.closing

Your code might look like this:

import httplib
import json
from contextlib import closing

def getReviewStatus(reviewboardUrl, id):
    try:
        with closing(httplib.HTTPConnection(reviewboardUrl, 80)) as conn:
            headers = {
                'Accept': 'application/json',
                'Content-Type': 'application/json; charset=UTF-8'
            }
            conn.request("GET", "/api/review-requests/%s/" % id, headers=headers)
            return json.loads(conn.getresponse().read())['review_request']['status']
    except:
        return ""

I find it's good to get into the habit of using with to open resources so don't have to worry about forgetting to close the connection.

share|improve this answer
    
Wow, this is exactly what i had in mind ! Thanks a bunch !! i knew about with but i didn't knew about from contextlib import closing, absolutely brilliant ! –  Willem D'haeseleer May 30 '13 at 17:35
1  
Out of interest, what would happen with this code if the requests throws an exception and the implicit close call throws an exception as well ? –  Willem D'haeseleer May 30 '13 at 17:39
    
That's a good question! This would be difficult to test. Maybe ask on Stack Overflow? I personally have never seen an exception thrown while trying to close a file/socket. Are you getting one, or just trying to code defensively? –  John Syrinek May 30 '13 at 18:01
    
just trying to be defensive :) –  Willem D'haeseleer May 30 '13 at 22:47
add comment

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.