First Python project - comments welcome!

[bruno.desthuilliers at gmail.com]

On 7 avr, 10:03, Paul Scott <psc... at uwc.ac.za> wrote:
> 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