Programming Idiomatic Code

[Nathan Harmston]

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="

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()
except e:
print e
os._exit(99)
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)
req = urllib2.Request(url)
try:
xml = parse(urllib2.urlopen(url)).getroot()
city = xml.findtext('user/city')
except e:
print e.reason
os._exit(99)
try:
city_count[city] += 1
except:
city_count[city] = 1
return city_count

if __name__=='__main__':
if len(sys.argv) is 1:
id_list = [ 42346, 77290, 729 ]
else:
try: id_list = read_id_file(sys.argv[1])
except e: print e
for k, v in generate_count(id_list).items():
print "%s: %i" %(k, v)

 

[Steve Lianoglou]

I don't know about idiomatic, but here's a quick list of somethings
I'd change.

1) In general, I don't think it's a good idea for a called function to
blow out the system on error. So, in this example, I wouldn't have
raised errors be caught in the same function only to call os.exit. I'd
either deal with the exception in some sane way, or have it bubble up
to the caller to have it then decide what it should do in the face of
the error.

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()
except e:
print e
os._exit(99)
return ids

Maybe use a list comprehension here (try/except clauses excluded in
reference to previous comment @ top)

def read_id_file(filename):
id_file = open(filename, 'r')
ids = [line.strip() for line in id_file.xreadlines()]
id_file.close()
return ids

If you're shooting for only Python 2.5, you can use ``with`` (see
http://docs.python.org/tut/node10.html#cleanup-with) to make that even
more concise.

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)
req = urllib2.Request(url)
try:
xml = parse(urllib2.urlopen(url)).getroot()
city = xml.findtext('user/city')
except e:
print e.reason
os._exit(99)
try:
city_count[city] += 1
except:
city_count[city] = 1
return city_count

I maybe wouldn't bail on the error raised when the ``parse`` function
is called ... I'd rather skip the error and try the next one (but
that's just my preference, not sure if it's good/bad style)

Also, instead of doing this:

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

I like using the dict.get() methods ... which would make that a one
liner:

city_count[city] = city_count.get(city, 0) + 1

Now I gotta get back to work, too ... good luck getting that job! ;-)

-steve

 

[attn.steven.kuo]

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="

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()
except e:
print e
os._exit(99)
return ids

The expression in the except clause should
evaluate to an object that "matches" the exception.

For example,

import sys

try:
idfile = open(filename, "r")
except IOError, e
print >> sys.stderr, str(e)
# or look into the warnings module
# etc.

 

[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

 

[Nathan Harmston]

HI,

Thanks to everyone for your comments ( i never knew "with" existed,
but to quote Borat "I like", unfortunately I cant apply for the job as
i m in the UK and dont have the experience but hey 10 minutes of
programming python beats 12 hours of programming in Clipper-derived
unreadable drivel (you dont know how much I appreciate Python atm).

I have one question though:

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...)

Why is this a bad idea?

Thanks in advance

Nathan

PS I am very ashamed I wrote:
if (len(sys.argv)) is 1 ......................
I would ask that this is never spoken of again, oh well
off to work I go

 

[Bruno Desthuilliers]

Nathan Harmston a écrit :

HI, (snip)
I have one question though:

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...)

Why is this a bad idea?

Don't you have any idea ?

 

[Douglas Woodrow]

On Tue, 3 Jul 2007 10:19:07, Nathan Harmston

i m in the UK and dont have the experience but hey 10 minutes of
programming python beats 12 hours of programming in Clipper-derived
unreadable drivel (you dont know how much I appreciate Python atm).

"Clipper-derived unreadable drivel"

I'm intrigued, what language are you working in?

Clipper v5 was a pretty impressive development language for 1990 - with
code blocks, a flexible pre-processor, garbage collection, exception
handling, decent speed and an API to allow easy integration with
routines written in c.

It doesn't have to be unreadable at all (unless it's written by someone
who thinks it is dBase).

 

[Nathan Harmston]

Using a module global for this kind of data is usually a bad idea

Don't you have any idea ?
--

Not really.....problem with access, using unneeded memory... I
grasping at straws here...

 

[Nathan Harmston]

"Clipper-derived unreadable drivel"

I'm intrigued, what language are you working in?

Clipper v5 was a pretty impressive development language for 1990 - with
code blocks, a flexible pre-processor, garbage collection, exception
handling, decent speed and an API to allow easy integration with
routines written in c.

It doesn't have to be unreadable at all (unless it's written by someone
who thinks it is dBase).

Whilst I ve never programmed in standard clipper this language is an
inhouse language (I would tell you the company but then they would
know I wasnt working) developed from Clipper. It has no exception
handling, no code blocks, the subroutines arent really subroutines and
dont return anything they just write to a global variable which you
access after the call. The speed is amazingly slow, it crawls. And
dont even start me on GOTOs....you try to make your code easy to read
and maintain but it just looks like you ve gone on a 2 day binge and
been ill over your VDU. Perl IMO is a 1 day binge problem.

They do use real clipper here and it sounds a lot better than this
bstard son. C integration, oh to do some real programming would be
amazing.

I think its just a poor implementation of Clipper and it makes it
appreciate the genius of python, the indentation, the ease of reading,
ease of maintaining....its so perfect.

Now if only I could convince them to move to
Python....................dreams.....

Nathan

 

[Bruno Desthuilliers]

Nathan Harmston a écrit :

Not really.....problem with access, using unneeded memory... I
grasping at straws here...

It makes generate_cound needlessly dependent on this global. It makes
code harder to read. It makes code harder to test. It makes code harder
to maintain. It makes code harder to reuse.

Ok, I understand this is probably not going to be a problem in this
particular case. But you asked how idiomatic your code was, and using
globals where not necessary is not pythonic.