Hi,
I'm new to Python and recently completed my first project. I used
wxPython with wxGlade to generate the GUI bits.The application seems to
work well, but I am entirely self-taught, so have undoubtedly committed a
number of howlers in terms of style, design, standards, best practice and
so forth.
Is there any kind soul here who would be willing to take a look at the
code and offer comments? The code is at:
<
http://dl.dropbox.com/u/6106778/bbc.py>
Okay, here goes.
# Globals
Title = 0
Episode = 1
Categories = 2
Channel = 3
PID = 4
Index = 5
The recommended PEP-8 style for names of constants is ALL_CAPS. Also,
if you just have a simple enumeration like this, you can avoid setting
specific values, which might otherwise lead to the temptation to use
the values in some places instead of the constant names. Just tell
your program how to generate them, and let it do the work:
TITLE, EPISODE, CATEGORIES, CHANNEL, PID, INDEX = range(6)
=====
# Error handling: Log to file, show message and abort
def ProcessError(text):
logging.exception(text)
dlg = wx.MessageDialog(None, text + " " + str(sys.exc_info()[1]) + \
"\nSee " + log + " for details.", "BBC Programmes", \
wx.ICON_ERROR|wx.OK)
dlg.ShowModal()
dlg.Destroy()
sys.exit()
In the more recent versions of wxPython, which I assume you're using,
dialogs provide context managers to handle their destruction. The
above would become:
def process_error(text):
logging.exception(text)
with wx.MessageDialog(...) as dlg:
dlg.ShowModal()
sys.exit()
The value of the context manager is that its __exit__ method (which
destroys the dialog) is guaranteed to be called when the with block
exits, even if an exception is raised inside of it. You'll note I
also renamed the function using the PEP-8 style for functions.
Another comment here is that the text of the dialog is a good
candidate for Python's string formatting feature. Instead of:
text + " " + str(sys.exc_info()[1]) + "\nSee " + log + " for details."
do:
"{} {}\nSee {} for details.".format(text, sys.exc_info()[1], log)
which is more legible and also avoids doing multiple string concatenations.
=====
class clsUtils():
The parentheses are redundant; this is equivalent to "class
clsUtils:". Note that in Python 2.x, classes defined without a base
class are old-style classes by default, which have been removed in
Python 3. It's recommended that you use new-style classes in your
code unless you have a good reason not to. You can accomplish this by
inheriting from object explicitly:
class Utils(object):
Note I also converted the class name to the PEP-8 CapWords convention
for classes, and I dropped the redundant 'cls' prefix.
My other comment on this class is that it has no state, and no methods
apart from __init__. You apparently only instantiate it in order to
execute the __init__ method, which seems to initialize some global
variables rather than initializing the class instance. If you don't
plan on interacting with the Utils instance as an object, then this
would make more sense as a function.
=====
def OnDescription(self, event): # wxGlade: MyFrame.<event_handler>
wx.BeginBusyCursor()
if Linux:
wx.Yield()
pos = self.TopGrid.GetGridCursorRow()
TitleEp = self.Prettify(recs[pos][Title], recs[pos][Episode])
self.TopFrame_statusbar.SetStatusText("Retrieving description
for " + TitleEp)
info = subprocess.check_output("get_iplayer --info " +
str(recs[pos][Index]), shell=True)
info = str.splitlines(info, False)
for line in info:
if line[:5] == "desc:":
info = line[16:]
break
wx.EndBusyCursor()
...
The BusyCursor is another resource that provides a context manager.
You can use it like this:
def on_description(self, event): # wxGlade: MyFrame.<event_handler>
with wx.BusyCursor():
...
This is a good idea since if an exception is raised in the middle of
the method, the mouse pointer won't end up stuck as an hourglass.
Also note that I once again meddled with the naming style to conform
with PEP-8, this time for the method name.
Further, this line:
info = str.splitlines(info, False)
could be written simply as:
info = info.splitlines(False)
=====
def OnIdle(self, event):
# Instantiate the other classes here, then hand over to TopFrame
if self.first_time:
self.first_time = False
...
An idle event handler is the wrong paradigm here. Idle events are for
background computation that you need to do regularly whenever the
application becomes idle. For the simpler case of a one-time function
call that should not run until after the event loop has started, use
the wx.CallAfter() function.
=====
I don't have any more specific observations. The only other thing I
would comment on is that you seem to be using a fair number of global
variables. Your components would be more readily reusable if you
would avoid using globals and stick to stateful objects instead.
Cheers,
Ian
