Programming Idiomatic Code

[Bruno Desthuilliers]

Nathan Harmston a écrit :
> Hi,
> 
> I m sorry but I m bored at work (and no ones looking so I can write
> some Python) and following a job advertisement post,I decided to write
> the code to do its for the one entitled Ninjas or something like that.
> I was wondering what could be done to my following code to make it
> more idiomatic...or whether it was idiomatic and to be honest what
> idiomatic really means. All comments greatly appreciated and welcomed.
> 
> Thanks in advance
> 
> Nathan
> 
> import urllib2,sys
> from elementtree.ElementTree import parse
> 
> base_url = "http://api.etsy.com/feeds/xml_user_details.php?id="

Using a module global for this kind of data is usually a bad idea 
(except eventually for run-once throw-away scripts, and even then...)

> def read_id_file(filename):
>    """ reads a file and generates a list of ids from it"""
>    ids = [ ]
>    try:
>     id_file = open(filename, "r")
>     for l in id_file:
>            ids.append( l.strip("\n") )
>     id_file.close()

Python has other idioms for this (list comprehensions etc).


>    except e:
>        print e
>        os._exit(99)

This is a very bad way to handle exceptions.
1/ you use a catch-all except clause, when you should be specific about 
what kind of exceptions you are willing to handle here
2/ names starting with an underscore are not part of the API. You should 
not use them unless you have a *very* compelling reason to do so.
3/ stdout is for normal outputs. Error messages and such should go to stderr
3/ anyway, if it's for printing the exception then exit, you'd better 
not handle anything - you'd have a similar result, but with a full 
traceback.

>    return ids

def read_id_file(filename):
   try:
     f = open(filename)
   except IOError, e:
     print >> sys.stderr, \
       "failed to open %s for reading : %s" \
        % (filename, e)
     return None
   else:
     ids = [l.strip() for l in f]
     f.close()
     return ids


> def generate_count(id_list):
>    """ takes a list of ids and returns a dictionary of cities with
> associated counts"""
>    city_count = {}
>    for i in id_list:
>        url = "%s%s" %(base_url,i)

base_url should be passed in as an argument.

>     req = urllib2.Request(url)

I assume there's a problem with indentation here...

>     try:
>            xml = parse(urllib2.urlopen(url)).getroot()
>            city  = xml.findtext('user/city')
>     except e:
>            print e.reason
>            os._exit(99)

cf above

>     try:
>            city_count[city] += 1
>        except:

should be 'except KeyError:'

>            city_count[city] = 1

This idiom is appropriate if you expect few exceptions - that is, if the 
normal case is that city_count.has_key(city) == True. Else, you'd better 
use an explicit test, a defaultdict, etc. The idiomatic expliciti test 
would be:

       if city not in city_count:
         city_count['city'] = 1
       else:
          city_count[city] += 1


>    return city_count
> 
> if __name__=='__main__':
>    if len(sys.argv) is 1:

'is' is the identity operator. *Never* use it for numeric equality test. 
This should be:
      if len(sys.argv) == 1:

>        id_list = [ 42346, 77290, 729 ]
>    else:
>        try: id_list = read_id_file(sys.argv[1])
>        except e: print e

cf above about exception handling.

And FWIW, here, I would have used a try/except :

    try:
      filename = sys.argv[1]
    except IndexError:
      id_list = [1, 2, 3]
      print >> sys.stderr, \
        "no id file passed, using default id_list %" % id_list
    else:
      print >> sys.stderr, \
        "reading id_list from id_file %s" % filename
      id_list = read_if_file(filename)

    if not id_list:
      sys.exit(1)

>    for k, v in generate_count(id_list).items():
>        print "%s: %i" %(k, v)

There's a simpler way:

      for item in generate_count(id_list).iteritems():
          print "%s: %i" % item