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
--
http://mail.python.org/mailman/listinfo/python-list
