critique my code, please

Discussion in 'Python' started by Brian Blais, Feb 6, 2006.

  1. Brian Blais

    Brian Blais Guest

    Hello,

    I am including at the end of this document (is it better as an attachment?) some code
    for a small gui dialog. Since I am quite new to this, if anyone has any suggestions
    for improvements to the code, bad coding practices, poor gui design, etc... I'd love
    to hear it. This list has been very helpful to me so far, and I hope to be able to
    return the favor someday when I get good enough to take the training wheels off. :)

    The code makes a simple class, with some parameters, some of which are numbers, some
    boolean, and one which is either a string or a number depending on context. There is
    a dialog class which allows you to edit/change the values, and a wrapper function of
    the form: new_params <== wrapper(old_params) which calls the dialog, and returns
    the updated params instance.

    thanks,

    Brian Blais

    --
    -----------------


    http://web.bryant.edu/~bblais

    import wx
    import copy

    class SimulationParams(object):

    def __init__(self):

    self.epoch_number=500
    self.iter_per_epoch=500
    self.epoch_per_display=1
    self.random_seed='clock'
    self.keep_every_epoch=0
    self.save_input_vectors=0

    def __repr__(self):

    yesno={0:"No",1:"Yes",True:"Yes",False:"No"}

    s="Epoch Number: %d\n" % self.epoch_number
    s=s+"Iter Per Epoch: %d\n" % self.iter_per_epoch
    s=s+"Epoch Per Display: %d\n" % self.epoch_per_display
    if (isinstance(self.random_seed,str)):
    s=s+"Random Seed: %s\n" % self.random_seed
    else:
    s=s+"Random Seed: %d\n" % self.random_seed

    s=s+"Keep Every Epoch: %s\n" % yesno[self.keep_every_epoch]
    s=s+"Save Input Vectors: %s\n" % yesno[self.save_input_vectors]
    return(s)


    class SimulationParamsDialog(wx.Dialog):

    def __init__(self,params,parent=None):

    self.params=params


    wx.Dialog.__init__(self, parent, -1, "Simulation Parameters")

    sizer = wx.BoxSizer(wx.VERTICAL)
    box = wx.BoxSizer(wx.HORIZONTAL)

    label = wx.StaticText(self, -1, "Epoch_Number:")
    box.Add(label, 0, wx.ALIGN_CENTRE|wx.ALL, 5)

    self.text1 = wx.TextCtrl(self, -1, str(params.epoch_number), size=(80,-1))
    box.Add(self.text1, 1, wx.ALIGN_CENTRE|wx.ALL, 5)

    sizer.Add(box, 0, wx.GROW|wx.ALIGN_CENTER_VERTICAL|wx.ALL, 5)

    box = wx.BoxSizer(wx.HORIZONTAL)
    label = wx.StaticText(self, -1, "Iterations Per Epoch:")
    box.Add(label, 0, wx.ALIGN_CENTRE|wx.ALL, 5)

    self.text2 = wx.TextCtrl(self, -1, str(params.iter_per_epoch), size=(80,-1))
    box.Add(self.text2, 1, wx.ALIGN_CENTRE|wx.ALL, 5)

    sizer.Add(box, 0, wx.GROW|wx.ALIGN_CENTER_VERTICAL|wx.ALL, 5)

    box = wx.BoxSizer(wx.HORIZONTAL)
    label = wx.StaticText(self, -1, "Epoch Per Display:")
    box.Add(label, 0, wx.ALIGN_CENTRE|wx.ALL, 5)

    self.text3 = wx.TextCtrl(self, -1, str(params.epoch_per_display), size=(80,-1))
    box.Add(self.text3, 1, wx.ALIGN_CENTRE|wx.ALL, 5)

    sizer.Add(box, 0, wx.GROW|wx.ALIGN_CENTER_VERTICAL|wx.ALL, 5)

    box = wx.BoxSizer(wx.HORIZONTAL)
    label = wx.StaticText(self, -1, "Random Seed:")
    box.Add(label, 0, wx.ALIGN_CENTRE|wx.ALL, 5)

    self.text4 = wx.TextCtrl(self, -1, str(params.random_seed), size=(80,-1))
    box.Add(self.text4, 1, wx.ALIGN_CENTRE|wx.ALL, 5)

    sizer.Add(box, 0, wx.GROW|wx.ALIGN_CENTER_VERTICAL|wx.ALL, 5)

    self.cb1 = wx.CheckBox(self, -1, "Keep Every Epoch")
    self.cb1.SetValue(params.keep_every_epoch)
    sizer.Add(self.cb1, 1, wx.GROW|wx.ALIGN_CENTRE|wx.ALL, 5)
    self.cb2 = wx.CheckBox(self, -1, "Save Input Vectors")
    self.cb2.SetValue(params.save_input_vectors)
    sizer.Add(self.cb2, 1, wx.GROW|wx.ALIGN_CENTRE|wx.ALL, 5)




    btnsizer = wx.StdDialogButtonSizer()

    btn = wx.Button(self, wx.ID_OK)
    btn.SetHelpText("The OK button completes the dialog")
    btn.SetDefault()
    btnsizer.AddButton(btn)

    btn = wx.Button(self, wx.ID_CANCEL)
    btn.SetHelpText("The Cancel button cnacels the dialog. (Cool, huh?)")
    btnsizer.AddButton(btn)
    btnsizer.Realize()


    sizer.Add(btnsizer, 1, wx.GROW|wx.ALIGN_CENTRE|wx.ALL, 5)
    self.SetSizer(sizer)
    sizer.Fit(self)


    def SetSimParams(params):

    new_params=copy.copy(params)

    dlg=SimulationParamsDialog(params)
    val=dlg.ShowModal()

    if val == wx.ID_OK:
    new_params.epoch_number=eval(dlg.text1.GetValue())
    new_params.iter_per_epoch=eval(dlg.text2.GetValue())
    new_params.epoch_per_display=eval(dlg.text3.GetValue())

    if (dlg.text4.GetValue()=='clock'):
    new_params.random_seed='clock'
    else:
    new_params.random_seed=eval(dlg.text4.GetValue())

    new_params.keep_every_epoch=dlg.cb1.GetValue()
    new_params.save_input_vectors=dlg.cb2.GetValue()

    print "ok"
    else:
    print "cancel"

    dlg.Destroy()

    return(new_params)

    if __name__ == '__main__':
    app = wx.PySimpleApp(0)

    params=SimulationParams()
    new_params=SetSimParams(params);

    print params
    print new_params
    app.MainLoop()
     
    Brian Blais, Feb 6, 2006
    #1
    1. Advertising

  2. Brian Blais

    Guest

    Just a few suggestions:

    1) use consistant formatting, preferably something like:
    http://www.python.org/peps/pep-0008.html
    E.g.:
    yesno = {0:"No", 1:"Yes", True:"Yes", False:"No"}

    2) if (isinstance(self.random_seed,str)):
    s=s+"Random Seed: %s\n" % self.random_seed
    else:
    s=s+"Random Seed: %d\n" % self.random_seed
    is unnecessary, since %s handles any type. Just say:
    s=s+"Random Seed: %s\n" % self.random_seed
    without any if statement.(unless you need fancy numeric formatting).

    3) I would strongly discourage using print statements (other than for
    debugging) in a GUI program. In my experience, users fire up the GUI
    and close (or kill!) the parent tty window, ignoring any dire messages
    printed on stdout or stderr. In a GUI app, errors, warnings, any
    message
    should be in popup dialogs or in a message bar in the main window.

    4) If you want to be cute, you can use
    s += 'more text'
    instead of
    s = s + 'more text'

    I'm not a wx user so I can't comment on the GUI implementation.
    Good luck!

    -- George
     
    , Feb 6, 2006
    #2
    1. Advertising

  3. Brian Blais asked for suggestions and critiques of his code.
    For style, look at:
    http://www.python.org/peps/pep-0008.html

    This is how I'd rewrite the first class:

    YESNO = ('No', 'Yes')

    class SimulationParams(object):
    '''Simulation Parameters setting up of a simulation'''
    def __init__(self):
    '''Build a new simulation control object'''
    self.epoch_number = 500
    self.iter_per_epoch = 500
    self.epoch_per_display = 1
    self.random_seed = 'clock'
    self.keep_every_epoch = 0
    self.save_input_vectors = 0

    def __repr__(self):
    return '''Epoch Number: %s
    Iter Per Epoch: %s
    Epoch Per Display: %s
    Random Seed: %s
    Keep Every Epoch: %s
    Save Input Vectors: %s
    ''' % (self.epoch_number, self.iter_per_epoch,
    self.epoch_per_display, self.random_seed,
    YESNO[self.keep_every_epoch],
    YESNO[self.save_input_vectors])

    Notes:
    The docstrings I am using are too generic. You know your app, so
    do something more app-appropriate.

    True is a version of 1, and False is a version of 0, so a dictionary
    like {0:"No",1:"Yes",True:"Yes",False:"No"} is actually length 2.
    I'd just use a constant mapping available anywhere.

    'a %s b' % 13 is 'a 13 b', so no need for your seed type test. If you
    do want to make the type visually obvious, use %r rather than %s for
    the seed.

    Personally I'd choose shorter names, but this is more taste:
    class SimulationParams(object):
    '''Simulation Parameters setting up of a simulation'''
    def __init__(self):
    '''Build a new simulation control object'''
    self.epoch = 500
    self.iter_per_epoch = 500
    self.epoch_per_display = 1
    self.seed = 'clock'
    self.keep_epochs = False # Since these seem to be booleans
    self.save_input = False # Since these seem to be booleans
    ...

    You also might consider making the __init__ provide defaulted args so
    getting a different initial setup only requires your changing the setup:

    ...
    def __init__(self, epoch=500, iter_per_epoch=500,
    epoch_per_display=1, seed='clock',
    keep_epochs=False, save_input=False):
    '''Build a new simulation control object'''
    self.epoch = epoch
    self.iter_per_epoch = iter_per_epoch
    self.epoch_per_display = epoch_per_display
    self.seed = seed
    self.keep_epochs = keep_epochs
    self.save_input = save_input
    ...


    More some other time.

    --Scott David Daniels
     
    Scott David Daniels, Feb 6, 2006
    #3
  4. "Brian Blais" <> wrote in message
    news:...
    > Hello,
    >
    > I am including at the end of this document (is it better as an

    attachment?) some code
    > for a small gui dialog. Since I am quite new to this, if anyone has any

    suggestions
    > for improvements to the code, bad coding practices, poor gui design,

    etc... I'd love
    > to hear it.


    The GUI is "welded" to the application.

    I much prefer to see a program split into a command-ling "engine"
    application and a (or more) "GUI", "CLI" e.t.c. interface applications that
    can connect to the engine and drive it. That way it is possible to script
    the application.
     
    Frithiof Andreas Jensen, Feb 8, 2006
    #4
    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. Michael Strorm
    Replies:
    26
    Views:
    784
    J. Campbell
    Nov 10, 2003
  2. Rv5

    Code Critique Please

    Rv5, Nov 16, 2003, in forum: C++
    Replies:
    3
    Views:
    368
    Benny Hill
    Nov 16, 2003
  3. gorda
    Replies:
    16
    Views:
    777
    Karl Heinz Buchegger
    Jul 29, 2004
  4. Adrian

    Code critique please

    Adrian, Oct 30, 2004, in forum: C++
    Replies:
    2
    Views:
    353
    Adrian
    Oct 31, 2004
  5. Rv5

    Code Critique Please

    Rv5, Nov 16, 2003, in forum: C Programming
    Replies:
    2
    Views:
    344
    -berlin.de
    Nov 16, 2003
Loading...

Share This Page