Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #18788 > unrolled thread

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

Back to article view | Back to comp.lang.python

Contents

  Newbie: Looking for code review on my first Python project. HoneyMonster <someone@someplace.invalid> - 2012-01-10 23:44 +0000
    Re: Newbie: Looking for code review on my first Python project. Ian Kelly <ian.g.kelly@gmail.com> - 2012-01-10 18:17 -0700
      Re: Newbie: Looking for code review on my first Python project. HoneyMonster <someone@someplace.invalid> - 2012-01-11 11:39 +0000
        Re: Newbie: Looking for code review on my first Python project. HoneyMonster <someone@someplace.invalid> - 2012-01-11 21:09 +0000
          Re: Newbie: Looking for code review on my first Python project. 88888 Dihedral <dihedral88888@googlemail.com> - 2012-01-11 16:24 -0800
    Re: Newbie: Looking for code review on my first Python project. Chris Angelico <rosuav@gmail.com> - 2012-01-11 12:43 +1100
      Re: Newbie: Looking for code review on my first Python project. Ben Finney <ben+python@benfinney.id.au> - 2012-01-11 13:05 +1100
    Re: Newbie: Looking for code review on my first Python project. Terry Reedy <tjreedy@udel.edu> - 2012-01-10 20:50 -0500
    Re: Newbie: Looking for code review on my first Python project. Terry Reedy <tjreedy@udel.edu> - 2012-01-10 22:59 -0500
      Re: Newbie: Looking for code review on my first Python project. Steven D'Aprano <steve+comp.lang.python@pearwood.info> - 2012-01-11 04:24 +0000
      Re: Newbie: Looking for code review on my first Python project. Ben Finney <ben+python@benfinney.id.au> - 2012-01-11 16:23 +1100

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

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

[toc] | [next] | [standalone]

#18792

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

[toc] | [prev] | [next] | [standalone]

#18821

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

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

[toc] | [prev] | [next] | [standalone]

#18845

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
>> <someone@someplace.invalid> 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

[toc] | [prev] | [next] | [standalone]

#18853

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
> >> <someone@someplace.invalid> 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.  

[toc] | [prev] | [next] | [standalone]

#18795

On Wed, Jan 11, 2012 at 10:44 AM, HoneyMonster
<someone@someplace.invalid> 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

[toc] | [prev] | [next] | [standalone]

#18798

Chris Angelico <rosuav@gmail.com> writes:

> On Wed, Jan 11, 2012 at 10:44 AM, HoneyMonster
> <someone@someplace.invalid> 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

Even better, you don't need to worry about what separator to use::

    top_dir = os.environ['HOME']
    log_filepath = os.path.join(top_dir, "log", "bbc.log")

> 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"""

Which you can get indented nicely in the source, and strip off the
indentation at run-time:

    import textwrap

    about = textwrap.dedent("""\
            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
            """)

>         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))

Which is a whole lot more readable using the recommendations in PEP 8::

    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))

-- 
 \       “I distrust those people who know so well what God wants them |
  `\    to do to their fellows, because it always coincides with their |
_o__)                      own desires.” —Susan Brownell Anthony, 1896 |
Ben Finney

[toc] | [prev] | [next] | [standalone]

#18796

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

[toc] | [prev] | [next] | [standalone]

#18802

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

[toc] | [prev] | [next] | [standalone]

#18803

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

[toc] | [prev] | [next] | [standalone]

#18808

Terry Reedy <tjreedy@udel.edu> writes:

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

Or use a trimple quoted string, with indentation in the source, and use
Python's batteries to manage it at runtime. Best of both worlds
<URL:http://stackoverflow.com/questions/2504411/proper-indentation-for-python-multiline-strings/2504454#2504454>.

-- 
 \     “I have an answering machine in my car. It says, ‘I'm home now. |
  `\  But leave a message and I'll call when I'm out.’” —Steven Wright |
_o__)                                                                  |
Ben Finney

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web