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.

I'm starting to look at Python and it's facilities. I just try to make one simple program using some base libraries.

I would like to hear some comments about anything you've noticed - style, readability, safety, etc.

#!/usr/bin/python2.6 -u

import urllib2
import simplejson
import zlib
from optparse import OptionParser

class StackOverflowFetcher:
    """Simple SO fetcher"""
    def getUserInfo( self, userId ):
        response = urllib2.urlopen( 'http://api.stackoverflow.com/1.1/users/' + str(userId) )
        response = response.read()
        jsonData = zlib.decompress( response, 16+zlib.MAX_WBITS )
        return simplejson.loads( jsonData )

    def getUserDisplayName( self, userId ):
        return self.getUserInfo( userId )['users'][0]['display_name']

    def getUserViewCount( self, userId ):
        return self.getUserInfo( userId )['users'][0]['view_count']

    def getUserReputation( self, userId ):
        return self.getUserInfo( userId )['users'][0]['reputation']


if __name__ == '__main__':
    parser = OptionParser()
    parser.add_option("-u", "--userId", dest="userId", help="Set userId (default is 1)", default=1 )
    parser.add_option("-n", "--display-name", action="store_true", dest="show_display_name", default=False, help="Show user's display name")
    parser.add_option("-v", "--view_count", action="store_true", dest="show_view_count", default=False, help="Show user's profile page view count")
    parser.add_option("-r", "--reputation", action="store_true", dest="show_reputation", default=False, help="Show user's reputation")
    (options, args) = parser.parse_args()

    userId = options.userId
    show_display_name = options.show_display_name
    show_view_count = options.show_view_count
    show_reputation = options.show_reputation

    if ( (not show_display_name) and (not show_view_count) and (not show_reputation) ):
        show_display_name = show_view_count = show_reputation = True

    fetcher = StackOverflowFetcher()
    if ( show_display_name ) : print fetcher.getUserDisplayName( userId )
    if ( show_view_count) : print fetcher.getUserViewCount( userId )
    if ( show_reputation ) : print fetcher.getUserReputation( userId )
share|improve this question

3 Answers 3

up vote 6 down vote accepted
#!/usr/bin/python2.6 -u

import urllib2
import simplejson
import zlib
from optparse import OptionParser

class StackOverflowFetcher:
    """Simple SO fetcher"""
    def getUserInfo( self, userId ):

The python style guide recommends words_with_underscores for function names and parameter names.

        response = urllib2.urlopen( 'http://api.stackoverflow.com/1.1/users/' + str(userId) )

I'd consider moving the API into a global constant

        response = response.read()
        jsonData = zlib.decompress( response, 16+zlib.MAX_WBITS )

I'd combine the previous two lines

        return simplejson.loads( jsonData )

    def getUserDisplayName( self, userId ):
        return self.getUserInfo( userId )['users'][0]['display_name']

    def getUserViewCount( self, userId ):
        return self.getUserInfo( userId )['users'][0]['view_count']

    def getUserReputation( self, userId ):
        return self.getUserInfo( userId )['users'][0]['reputation']

There three functions share most of their code. Why don't you modify getUserInfo to do the ['users'][0] itself instead?

if __name__ == '__main__':
    parser = OptionParser()
    parser.add_option("-u", "--userId", dest="userId", help="Set userId (default is 1)", default=1 )
    parser.add_option("-n", "--display-name", action="store_true", dest="show_display_name", default=False, help="Show user's display name")
    parser.add_option("-v", "--view_count", action="store_true", dest="show_view_count", default=False, help="Show user's profile page view count")
    parser.add_option("-r", "--reputation", action="store_true", dest="show_reputation", default=False, help="Show user's reputation")
    (options, args) = parser.parse_args()

    userId = options.userId
    show_display_name = options.show_display_name
    show_view_count = options.show_view_count
    show_reputation = options.show_reputation

    if ( (not show_display_name) and (not show_view_count) and (not show_reputation) ):

None of those parentheses are necessary. I'd probably go with if not (show_display_name or show_view_count or show_reputation): as I think that mostly clear states what you are doing. show_display_name = show_view_count = show_reputation = True

    fetcher = StackOverflowFetcher()
    if ( show_display_name ) : print fetcher.getUserDisplayName( userId )
    if ( show_view_count) : print fetcher.getUserViewCount( userId )
    if ( show_reputation ) : print fetcher.getUserReputation( userId )

Firstly, the ifs don't need the parens. Secondly, the way your code is written each call goes back to the server. So you'll three calls to the stackoverflow server which you could have gotten with one.

share|improve this answer

I think your naming scheme is good and on first sight its very easy to see what the code does without having to study it too much.

However, if I would pick on something I would say:

  • Remove the unnecessary whitespace, ex. if ( condition ), or self.getUserInfo( userId ). This is of course a matter of taste, but I find it more consistent with regular coding style to not 'bloat' with whitespace.

  • optparser was deprecated in 2.7: http://docs.python.org/library/optparse.html

  • in parser.add.option() you could remove the word "show" from the dest name and rather do the following, hence eliminating the need for all the declarations of show_display_name and still keep easy code readability.

    ...
    parser.add.option(parser.add_option("-r", "--reputation", action="store_true", dest="reputation", default=False, help="Show user's reputation")
    (show, args) = parser.parse_args()
    
    fetch = StackOverflowFetcher()
    if(show.reputation) : print fetch.getUserReputation(show.userId)
    
  • I don't really see what the point of the following line which sets everything to true, it seems a bit weird since you use store_true as action if the option parsed is set.

    if ( (not show_display_name) and (not show_view_count) and (not show_reputation) ):
    show_display_name = show_view_count = show_reputation = True
    
share|improve this answer
    
I could removing "show" from dest names, but then what about userId option? –  ДМИТРИЙ МАЛИКОВ Jan 18 '12 at 21:30
    
You could just use show.UserId in fetch.getUserReputation. It doesn't break consistency in any major way, you are just showing/providing UserId to get getUserReputation method. If its very important for you, you could always do as you did at first: userId = show.userId, but again, I don't really see the need. –  bleakgadfly Jan 19 '12 at 10:04

http://api.stackoverflow.com/1.1/users/string_id returns

{
  "error": {
    "code": 404,
    "message": "The server has not found anything matching the Request-URI."
  }
}

and will raise KeyErrors here:

def getUserDisplayName( self, userId ):
    return self.getUserInfo( userId )['users'][0]['display_name']

def getUserViewCount( self, userId ):
    return self.getUserInfo( userId )['users'][0]['view_count']

def getUserReputation( self, userId ):
    return self.getUserInfo( userId )['users'][0]['reputation']

http://api.stackoverflow.com/1.1/users/9924 returns

{
  "total": 0,
  "page": 1,
  "pagesize": 30,
  "users": []
}

and will raise IndexErrors here:

def getUserDisplayName( self, userId ):
    return self.getUserInfo( userId )['users'][0]['display_name']

def getUserViewCount( self, userId ):
    return self.getUserInfo( userId )['users'][0]['view_count']

def getUserReputation( self, userId ):
    return self.getUserInfo( userId )['users'][0]['reputation']

And since you are using userId as an argument to every method, and the StackOverflowFetcher instance is used only for 1 userId – it might me a good idea to add __init__ method:

__init__(self, userId):
    # some userId validation
    self.userId = userId

and save yourself a bit of passing userId around.


UPD:


If all options are set to True, this will call getUserInfo and, therefore, query api 3 times:

if ( show_display_name ) : print fetcher.getUserDisplayName( userId )
if ( show_view_count) : print fetcher.getUserViewCount( userId )
if ( show_reputation ) : print fetcher.getUserReputation( userId )

Since you call it at least once any way, you better call it in the __init__(), or just store retrieved value in an instance attribute and use it like this:

def __init__(self, userId):
    #...
    self.userInfo = None

def getUserReputation(self):
    if self.userInfo is None:
        self.userInfo = self.getUserInfo()

    return self.userInfo['users'][0]['reputation']
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.