Newbie: Looking for code review on my first Python project.

Discussion in 'Python' started by HoneyMonster, Jan 10, 2012.

  1. HoneyMonster

    HoneyMonster Guest

    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>

    Thanks
    HoneyMonster, Jan 10, 2012
    #1
    1. Advertising

  2. HoneyMonster

    Ian Kelly Guest

    On Tue, Jan 10, 2012 at 4:44 PM, HoneyMonster <> wrote:
    > 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
    Ian Kelly, Jan 11, 2012
    #2
    1. Advertising

  3. On Wed, Jan 11, 2012 at 10:44 AM, HoneyMonster
    <> wrote:
    > 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.


    Welcome!

    Ian has already offered some excellent tips, so I'll not repeat him.


    log = os.environ['HOME'] + "/log/bbc.log"
    log = os.environ['HOMEPATH'] + "\\log\\bbc.log"

    Python on Windows will support / for paths; I'd then break out the
    HOME / HOMEPATH check into a separate variable (eg 'basepath'), and
    then only that and icondir would need to be in the 'if Linux' block.


    about = "Built by Walter Hurry using Python and wxPython,\n" + \
    "with wxGlade to generate the code for the GUI elements.\n" + \
    "Phil Lewis' get_iplayer does the real work.\n\n" + \
    "Version 1.05: January 10, 2012"

    I'd do this with a triple-quoted string:

    about = """Built by Walter Hurry using Python and wxPython,
    with wxGlade to generate the code for the GUI elements.
    Phil Lewis' get_iplayer does the real work.

    Version 1.05: January 10, 2012"""


    # Configure the logging
    # Generate a list for the PVR queue

    Comments like this aren't much use, although the second would be a
    good place to expand "PVR".

    # We only want the PID at the start if the line, and
    it is always 8 bytes

    Much more useful :)


    self.add = wx.MenuItem(self.file, wx.NewId(), "&Add to Queue",
    "Add a programme to the queue (for download later)", wx.ITEM_NORMAL)
    self.file.AppendItem(self.add)

    I don't have/use wxpython so I can't say for sure, but I think
    AppendItem returns the item appended. This allows you to avoid
    repeating yourself:

    self.add = self.file.AppendItem(wx.MenuItem(self.file,
    wx.NewId(), "&Add to Queue", "Add a programme to the queue (for
    download later)", wx.ITEM_NORMAL))

    This prevents mismatching assignment and append, when you copy/paste/edit etc.

    Decent bit of code. I've seen far worse... and not from new programmers :)

    Chris Angelico
    Chris Angelico, Jan 11, 2012
    #3
  4. HoneyMonster

    Terry Reedy Guest

    On 1/10/2012 8:17 PM, Ian Kelly wrote:
    > On Tue, Jan 10, 2012 at 4:44 PM, HoneyMonster<> wrote:
    >> 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. :)


    Nice review. Though OP used and you wrote about wx, when I get deeper
    into the IDLE code, I will look to see whether tkinter/idle resource
    provide context managers (if not, try to add) and whether idle is using
    them. (I won't be surprised if answers are often no and no.)

    --
    Terry Jan Reedy
    Terry Reedy, Jan 11, 2012
    #4
  5. HoneyMonster

    Terry Reedy Guest

    On 1/10/2012 8:43 PM, Chris Angelico wrote:

    > about = "Built by Walter Hurry using Python and wxPython,\n" + \
    > "with wxGlade to generate the code for the GUI elements.\n" + \
    > "Phil Lewis' get_iplayer does the real work.\n\n" + \
    > "Version 1.05: January 10, 2012"
    >
    > I'd do this with a triple-quoted string:
    >
    > about = """Built by Walter Hurry using Python and wxPython,
    > with wxGlade to generate the code for the GUI elements.
    > Phil Lewis' get_iplayer does the real work.


    I would too, but if you prefer the indentation, just leave out the '+'s
    and let Python do the catenation when compiling:
    >>> s = "abc\n" \

    "def\n"\
    "ghi"
    >>> s

    'abc\ndef\nghi'

    --
    Terry Jan Reedy
    Terry Reedy, Jan 11, 2012
    #5
  6. On Tue, 10 Jan 2012 22:59:23 -0500, Terry Reedy wrote:

    > On 1/10/2012 8:43 PM, Chris Angelico wrote:
    >
    >> about = "Built by Walter Hurry using Python and wxPython,\n" + \
    >> "with wxGlade to generate the code for the GUI elements.\n" +
    >> \ "Phil Lewis' get_iplayer does the real work.\n\n" + \
    >> "Version 1.05: January 10, 2012"
    >>
    >> I'd do this with a triple-quoted string:
    >>
    >> about = """Built by Walter Hurry using Python and wxPython, with
    >> wxGlade to generate the code for the GUI elements. Phil Lewis'
    >> get_iplayer does the real work.

    >
    > I would too, but if you prefer the indentation, just leave out the '+'s
    > and let Python do the catenation when compiling:
    > >>> s = "abc\n" \

    > "def\n"\
    > "ghi"
    > >>> s

    > 'abc\ndef\nghi'


    Actually, in recent Pythons (2.5 or better, I believe) the peephole
    optimizer will do the concatenation at compile time even if you leave the
    + signs in, provided that the parts are all literals.


    >>> from dis import dis
    >>> dis(compile(r's = "a\n" + "b\n"', '', 'single'))

    1 0 LOAD_CONST 3 ('a\nb\n')
    3 STORE_NAME 0 (s)
    6 LOAD_CONST 2 (None)
    9 RETURN_VALUE


    --
    Steven
    Steven D'Aprano, Jan 11, 2012
    #6
  7. HoneyMonster

    HoneyMonster Guest

    On Tue, 10 Jan 2012 18:17:48 -0700, Ian Kelly wrote:

    > On Tue, Jan 10, 2012 at 4:44 PM, HoneyMonster
    > <> wrote:
    >> 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.

    < snip constructive and helpful advice >

    Very many thanks to Ian and to all who responded. I really appreciate the
    guidance. Cheers.

    WH
    HoneyMonster, Jan 11, 2012
    #7
  8. HoneyMonster

    HoneyMonster Guest

    On Wed, 11 Jan 2012 11:39:48 +0000, HoneyMonster wrote:

    > On Tue, 10 Jan 2012 18:17:48 -0700, Ian Kelly wrote:
    >
    >> On Tue, Jan 10, 2012 at 4:44 PM, HoneyMonster
    >> <> wrote:
    >>> 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.

    > < snip constructive and helpful advice >
    >
    > Very many thanks to Ian and to all who responded. I really appreciate
    > the guidance. Cheers.



    I have taken on board the helpful suggestions offered, and looked though
    the PEP-8 document which has been mentioned.

    As a result, there are a number of changes to the code. My second attempt
    is in the same place:

    <http://dl.dropbox.com/u/6106778/bbc.py>

    A couple of points:

    1) I'm reluctant to try to improve this bit of code:
    -------------------------------------------------------------
    self.add = wx.MenuItem(self.file, wx.NewId(), "&Add to Queue",
    "Add a programme to the queue (for download later)", wx.ITEM_NORMAL)
    self.file.AppendItem(self.add)
    -------------------------------------------------------------
    since it is generated by wxGlade and so will be overwritten.

    2) I was very unsure about the wx.CallAfter, and suspect that I have put
    it in the wrong place. It seems to pass off well enough in Linux, but on
    Windows it appears to prevent the widgets on the splash frame being drawn
    properly.

    If anyone would be kind enough, further comments would be welcomed.

    Thanks,
    WH
    HoneyMonster, Jan 11, 2012
    #8
  9. HoneyMonsteræ–¼ 2012å¹´1月12日星期四UTC+8上åˆ5時09分13秒寫é“:
    > On Wed, 11 Jan 2012 11:39:48 +0000, HoneyMonster wrote:
    >
    > > On Tue, 10 Jan 2012 18:17:48 -0700, Ian Kelly wrote:
    > >
    > >> On Tue, Jan 10, 2012 at 4:44 PM, HoneyMonster
    > >> <> wrote:
    > >>> 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.

    > > < snip constructive and helpful advice >
    > >
    > > Very many thanks to Ian and to all who responded. I really appreciate
    > > the guidance. Cheers.

    >
    >
    > I have taken on board the helpful suggestions offered, and looked though
    > the PEP-8 document which has been mentioned.
    >
    > As a result, there are a number of changes to the code. My second attempt
    > is in the same place:
    >
    > <http://dl.dropbox.com/u/6106778/bbc.py>
    >
    > A couple of points:
    >
    > 1) I'm reluctant to try to improve this bit of code:
    > -------------------------------------------------------------
    > self.add = wx.MenuItem(self.file, wx.NewId(), "&Add to Queue",
    > "Add a programme to the queue (for download later)", wx.ITEM_NORMAL)
    > self.file.AppendItem(self.add)
    > -------------------------------------------------------------
    > since it is generated by wxGlade and so will be overwritten.
    >
    > 2) I was very unsure about the wx.CallAfter, and suspect that I have put
    > it in the wrong place. It seems to pass off well enough in Linux, but on
    > Windows it appears to prevent the widgets on the splash frame being drawn
    > properly.
    >
    > If anyone would be kind enough, further comments would be welcomed.
    >
    > Thanks,
    > WH


    I haven't tried wxGlade for several years. I checked BOA, WxGlade and Wxpython
    and pygame 4 years ago. Auto code generators in BOA and WxGlade are more helpful
    to python programmers.

    One can develop GUI by python with Tcl/tk or Qt, too.
    But the license conditions in software packages are not all the same.
    88888 Dihedral, Jan 12, 2012
    #9
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Ed Thompson
    Replies:
    0
    Views:
    641
    Ed Thompson
    Oct 20, 2003
  2. www
    Replies:
    51
    Views:
    1,478
  3. Andrea Francia
    Replies:
    2
    Views:
    248
    Steven D'Aprano
    Jan 6, 2009
  4. Ben Schaffhausen
    Replies:
    5
    Views:
    153
    John Labovitz
    Sep 21, 2006
  5. Brian A.
    Replies:
    4
    Views:
    139
    Martin DeMello
    Nov 28, 2008
Loading...

Share This Page