I have started, and made some progress (OK it works, but needs some
love) on my first real Python application.
http://cvs2.uwc.ac.za/trac/python_tools/browser/podder
I would love some feedback on what I have done. In total this has taken
me 5 nights to do (I am working on it at night as PHP, not Python, is my
day job), so it can probably do with *lots* of improvement. All code is
GPL.
If anyone on this list is willing/able, please do give me a few
pointers, even if it is "This is total crap - RTFM and come back when
you are ready" I would really appreciate it!
Ok, since you asked for it:
22 try:
23 import pygtk
24 #tell pyGTK, if possible, that we want GTKv2
25 pygtk.require("2.0")
26 except:
Don't use bare except clauses, always mention the exception class
you're expecting and let every other exception propagate. Else you may
shadow other unexpected errors (ie: what if the user has a pygtk.py
file in her pythonpath that is unrelated to the expected pygtk
module ? And yes, this kind of thing can and does happen, more often
than you may think).
27 print "You need to install pyGTK or GTKv2 or set your
PYTHONPATH correctly"
stdout is for normal program outputs. Error messages should go to
sys.stderr.
28 print "try: export PYTHONPATH=/usr/local/lib/python2.2/site-
packages/"
29 sys.exit(1)
40 class appgui:
1/ naming convention : should be Appgui or AppGui (cf pep08)
2/ unless you have very compelling reasons to stick with old-style
classes, make your class a newstyle one:
class AppGui(object):
41 def __init__(self):
42 """
43 In this init we are going to display the main recorder
window
44 """
45 global globaldir
46 globaldir="./"
Since this doesn't depend on anything passed to the initializer, and
is initialized with a constant value, this should go to the top-level,
ie:
GLOBAL_DIR = './'
class AppGui(object):
# code here
58 "on_window1_destroy" : (gtk.main_quit)}
This may not do what you think. If what you want is to pass a tuple,
the syntax is:
"on_window1_destroy" : (gtk.main_quit, )
}
notice the trailing ','
59 self.wTree.signal_autoconnect (dic)
60 self.status = STOPPED
61 return
You don't need this return statement.
64 def record_click(self,widget):
(snip)
70 self.player = gst.Pipeline("recorder")
(snip)
87 def pause_click(self,widget):
(snip)
94 if widget.get_active():
95 # print "paused recording..."
96 self.player.set_state(gst.STATE_PAUSED)
This may be a bit of a personnal preference, but I never feel
confortable with attributes added in a method (I mean, else than the
initializer) and accessed in another. Specially when it's a very
important attribute... One reason being that I don't get the 'big
picture' just from browsing the __init__ and the methods names,
another being that now pause_click depends on record_click having been
called before - yet nothing mentions it, nothing documents it, you
have to read the whole code to find about it.
204 try:
205 f=open(globaldir+"lecture.ogg", 'r')
Steve Holden already commented on using os.path.join here, and I
commented about using a top-level constant (GLOBAL_DIR), which will
makes thing more explicit (we know it's a constant, and we will look
for it at the top-level). I'd recommend to also use top-level
symbolic constants for the file names, instead of hardcoding them in
the methods. Same thing for urls etc.
206 b=open(globaldir+"basefile", 'w')
207 encoded = base64.encode(f,b)
208 b.close()
209 except:
210 print "File could not be opened..."
And what you get an exception in the call to base64.encode ? This is
*exactly* why you should *never* use bare except clauses.
(snip more bare except clauses...)
And while we're at it, you forget to close f.
283 if type(currentThread()) != _MainThread:
Since types are singletons, you can use the identity test here:
if type(currentThread()) is not _MainThread:
306 def __init__ (self, parrent, queue, signal,
sendPolicy):
(snip)
309 self.parrent = parrent
Don't you mean 'parent' ?-)
316 v = self.queue.get()
317 if v == None:
identity test again (more idiomatic, and a little bit faster)
318 break
319 threads_enter()
320 l = [v]
'v' is a very poor name, and 'l' is even worse.
431 # Get
stuff
#
432 def isCancelled (self):
433 return self.cancelled
434
435 def isDone (self):
436 return self.done
437
438 def getProgress (self):
439 return self.progress
Unless something in the framework requires these getters (I have
almost no experience with pyGTK), you'd be better using direct
attribute access IMHO. Else, it would be better to mark cancelled,
done and progress as implementation attributes (by prefixing them with
a single underscore) so everyone knows he shouldn't access them
directly.
Else it looks mostly clean !-)
HTH
