Stack Overflow user info fetcher

Question

I'm starting to look at Python and its facilities. I've just tried 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 )

Python version 2.6

Answer 1

#!/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.

Answer 2

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
  

• ДМИТРИЙ МАЛИКОВ: I could removing "show" from dest names, but then what about userId option?
• implmentor: 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.

Answer 3

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']