critique my code, please

[Brian Blais]

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

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

(e-mail address removed)
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()

 

[gry]

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

 

[Scott David Daniels]

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
(e-mail address removed)

 

[Frithiof Andreas Jensen]

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.