?

Log in

No account? Create an account
Python - brad's life [entries|archive|friends|userinfo]
Brad Fitzpatrick

[ website | bradfitz.com ]
[ userinfo | livejournal userinfo ]
[ archive | journal archive ]

Python [Aug. 15th, 2007|02:41 am]
Brad Fitzpatrick
[Tags|, , ]

Please criticize my first Python program.

I really should read a book. What's everybody's favorites?
LinkReply

Comments:
Page 1 of 2
<<[1] [2] >>
[User Picture]From: dimrub
2007-08-15 10:24 am (UTC)
You will do fine (actually, you do do fine) with the stuff on the Python's site. Being a partisan, I will however recommend the "Python's cookbook", for which a friend of mine wrote a chapter or two.
(Reply) (Thread)
[User Picture]From: ciphergoth
2007-08-15 10:29 am (UTC)
The only thing I can see to criticize at first glance is that it might be worth checking whether the exception is one you expect before ignoring it.

Also, if this is Python 2.5, look at using "with" to simplify your use of files:

http://docs.python.org/whatsnew/pep-343.html
(Reply) (Thread)
From: ext_51207
2007-08-15 10:40 am (UTC)
Dive into Python (http://www.diveintopython.org/) by Mark Pilgrim is a must read!
I also recommend checking out his blog, good stuff.
(Reply) (Thread)
From: node
2007-08-15 10:54 am (UTC)
file is a builtin, like open. You probably shouldn't use it as a variable for the same reason you're not using list as a variable.

You can iterate over dicts with for k, v in data.items(). You could del data[k] after the unlink.
(Reply) (Thread)
From: node
2007-08-15 10:55 am (UTC)

also, try using

pylint (should be in Debian).
(Reply) (Thread)
[User Picture]From: ghewgill
2007-08-15 11:00 am (UTC)

If you're iterating through a collection and deleting stuff, you ran into the obvious problem. The pythonic solution to that is something like:

for k in data.keys():
    if condition:
        del data[k]

That way you don't have to fiddle with todel.

(Reply) (Thread)
From: trs80 [typekey.com]
2007-08-15 01:35 pm (UTC)
for k in data[:]:
is another way I saw recently.

Back to the program, the biggest problem I have with mailman (although it's probably true of most mailing list managers) is that there's not much of a public API apart from a few command-line programs. Of course, I just went at looked at the docs, and bin/withlist provides exactly what I want.
(Reply) (Parent) (Thread) (Expand)
From: (Anonymous)
2007-08-15 11:12 am (UTC)

Python Links...

This is funny. Mostly because it's true:

http://dis.4chan.org/read/prog/1180084983/

This has nice coverage of Python's functional support:

http://gnosis.cx/TPiP/

Some interesting content in these videos:

http://video.google.com/videoplay?docid=-3035093035748181693
http://video.google.com/videoplay?docid=-288473283307306160

-ryan
www.asciiarmor.com
(Reply) (Thread)
From: amoffat
2007-08-15 11:15 am (UTC)
list comprehensions are (trivially) cooler than the filter builtin

lists = filter(os.path.isdir, os.listdir("."))
lists = [d for d in os.listdir(".") if os.path.isdir(d)]

might want to throw your top-level code in a def main() and add a if __name__ == '__main__': main() to the top level. makes for easier organization (and makes sure that if your code is imported, it isn't run automatically, not that it matters here in this instance)

you can do type(k) != int, because int is a type (along with str, float, etc). saves an import and some typing

other than that, not bad for a first python program. maybe you could critique my first perl program when i get around to writing it :)
(Reply) (Thread)
From: (Anonymous)
2007-08-15 12:04 pm (UTC)

pythonic style

If you want some advice regarding the "pythonic" style you can take a look here:

http://www.python.org/dev/peps/pep-0008/
http://python.net/~goodger/projects/pycon/2007/idiomatic/handout.html

Regarding your first program, I suggest you use isinstance instead of checking directly the type (see pep-8 also).

>>> type(1) != types.IntType
False
>>> not isinstance(1, int)
False

ciphergoth is right regarding the with statement and, as ghewgill said, you can improve the way you're working with the data dictionary. You can modify it during the iteration if you use the keys() or items() methods (just be sure you're not using one of the iter* equivalents). I would probably use data.items() instead of data.keys() so you have direct access to the key and it's value.

Favorites books? Dive Into Python it's free and great (only lack is that it covers python 2.2 IIRC), Learning Python and Python in a Nutshell have been updated to 2.5 I think. The python tutorial is also a great source.

Great to see you stepping on python. :-)
(Reply) (Thread)
From: (Anonymous)
2007-08-15 12:13 pm (UTC)
The Laughing Jesus by Timothy Freke!
(Reply) (Thread)
From: seeds_of_peace
2007-08-15 12:14 pm (UTC)
The Laughing Jesus by Timothy Freke!
(Reply) (Thread)
[User Picture]From: gnuvince
2007-08-15 12:40 pm (UTC)
Dive Into Python is generally recommended for people who know how to program and want to learn Python without having to read about what a variable or a string is.
(Reply) (Thread)
[User Picture]From: muerte
2007-08-15 01:47 pm (UTC)
(Reply) (Thread)
[User Picture]From: codetoad
2007-08-15 03:09 pm (UTC)
Quick things I noticed:

Docstrings are the convention for whole-program and whole-function notes. This means you want:

#!/usr/bin/python

"""
Remove moderation items from GNU Mailman's pending queue.
...
"""


Also, as kinda noted below, if you do:

if __name__ == '__main__':
for lst in lists:
    print "Cleaning list %s ..." % lst
    clean_list(lst)


you'll be able to use the program as a module ( without executing the main stuff automatically.
(Reply) (Thread)
[User Picture]From: avatraxiom
2007-08-15 03:27 pm (UTC)
Generally you want to use instanceof instead of comparing types directly. I'm pretty sure this applies to built-in types as well, as it's a problem most commonly run into with string types (where ascii strings have one type, and unicode strings have a "unicode" type, but they both inherit from basestring). It's like using ->isa instead of "blessed($ref) eq".

The filter could alternately have been written as a list comprehension, but it's actually probably clearer as a filter (which is kind of unusual). If you're curious, I think the list comprehension would have been:

[x for x in os.listdir(".") if os.path.isdir(x)]

-Max
(Reply) (Thread)
Page 1 of 2
<<[1] [2] >>