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_par ams) which calls the dialog, and returns
the updated params instance.

thanks,

Brian Blais

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

bblais@bryant.e du

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

import wx
import copy

class SimulationParam s(object):

def __init__(self):

self.epoch_numb er=500
self.iter_per_e poch=500
self.epoch_per_ display=1
self.random_see d='clock'
self.keep_every _epoch=0
self.save_input _vectors=0

def __repr__(self):

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

s="Epoch Number: %d\n" % self.epoch_numb er
s=s+"Iter Per Epoch: %d\n" % self.iter_per_e poch
s=s+"Epoch Per Display: %d\n" % self.epoch_per_ display
if (isinstance(sel f.random_seed,s tr)):
s=s+"Random Seed: %s\n" % self.random_see d
else:
s=s+"Random Seed: %d\n" % self.random_see d

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 SimulationParam sDialog(wx.Dial og):

def __init__(self,p arams,parent=No ne):

self.params=par ams


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

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

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

self.text1 = wx.TextCtrl(sel f, -1, str(params.epoc h_number), size=(80,-1))
box.Add(self.te xt1, 1, wx.ALIGN_CENTRE |wx.ALL, 5)

sizer.Add(box, 0, wx.GROW|wx.ALIG N_CENTER_VERTIC AL|wx.ALL, 5)

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

self.text2 = wx.TextCtrl(sel f, -1, str(params.iter _per_epoch), size=(80,-1))
box.Add(self.te xt2, 1, wx.ALIGN_CENTRE |wx.ALL, 5)

sizer.Add(box, 0, wx.GROW|wx.ALIG N_CENTER_VERTIC AL|wx.ALL, 5)

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

self.text3 = wx.TextCtrl(sel f, -1, str(params.epoc h_per_display), size=(80,-1))
box.Add(self.te xt3, 1, wx.ALIGN_CENTRE |wx.ALL, 5)

sizer.Add(box, 0, wx.GROW|wx.ALIG N_CENTER_VERTIC AL|wx.ALL, 5)

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

self.text4 = wx.TextCtrl(sel f, -1, str(params.rand om_seed), size=(80,-1))
box.Add(self.te xt4, 1, wx.ALIGN_CENTRE |wx.ALL, 5)

sizer.Add(box, 0, wx.GROW|wx.ALIG N_CENTER_VERTIC AL|wx.ALL, 5)

self.cb1 = wx.CheckBox(sel f, -1, "Keep Every Epoch")
self.cb1.SetVal ue(params.keep_ every_epoch)
sizer.Add(self. cb1, 1, wx.GROW|wx.ALIG N_CENTRE|wx.ALL , 5)
self.cb2 = wx.CheckBox(sel f, -1, "Save Input Vectors")
self.cb2.SetVal ue(params.save_ input_vectors)
sizer.Add(self. cb2, 1, wx.GROW|wx.ALIG N_CENTRE|wx.ALL , 5)




btnsizer = wx.StdDialogBut tonSizer()

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

btn = wx.Button(self, wx.ID_CANCEL)
btn.SetHelpText ("The Cancel button cnacels the dialog. (Cool, huh?)")
btnsizer.AddBut ton(btn)
btnsizer.Realiz e()


sizer.Add(btnsi zer, 1, wx.GROW|wx.ALIG N_CENTRE|wx.ALL , 5)
self.SetSizer(s izer)
sizer.Fit(self)


def SetSimParams(pa rams):

new_params=copy .copy(params)

dlg=SimulationP aramsDialog(par ams)
val=dlg.ShowMod al()

if val == wx.ID_OK:
new_params.epoc h_number=eval(d lg.text1.GetVal ue())
new_params.iter _per_epoch=eval (dlg.text2.GetV alue())
new_params.epoc h_per_display=e val(dlg.text3.G etValue())

if (dlg.text4.GetV alue()=='clock' ):
new_params.rand om_seed='clock'
else:
new_params.rand om_seed=eval(dl g.text4.GetValu e())

new_params.keep _every_epoch=dl g.cb1.GetValue( )
new_params.save _input_vectors= dlg.cb2.GetValu e()

print "ok"
else:
print "cancel"

dlg.Destroy()

return(new_para ms)

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

params=Simulati onParams()
new_params=SetS imParams(params );

print params
print new_params
app.MainLoop()

[gry@ll.mit.edu]

Re: critique my code, please

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(sel f.random_seed,s tr)):
s=s+"Random Seed: %s\n" % self.random_see d
else:
s=s+"Random Seed: %d\n" % self.random_see d
is unnecessary, since %s handles any type. Just say:
s=s+"Random Seed: %s\n" % self.random_see d
without any if statement.(unle ss 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]

Re: critique my code, please

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 SimulationParam s(object):
'''Simulation Parameters setting up of a simulation'''
def __init__(self):
'''Build a new simulation control object'''
self.epoch_numb er = 500
self.iter_per_e poch = 500
self.epoch_per_ display = 1
self.random_see d = '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_num ber, self.iter_per_e poch,
self.epoch_per_ display, self.random_see d,
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",Fal se:"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 SimulationParam s(object):
'''Simulation Parameters setting up of a simulation'''
def __init__(self):
'''Build a new simulation control object'''
self.epoch = 500
self.iter_per_e poch = 500
self.epoch_per_ display = 1
self.seed = 'clock'
self.keep_epoch s = 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_displ ay=1, seed='clock',
keep_epochs=Fal se, save_input=Fals e):
'''Build a new simulation control object'''
self.epoch = epoch
self.iter_per_e poch = iter_per_epoch
self.epoch_per_ display = epoch_per_displ ay
self.seed = seed
self.keep_epoch s = keep_epochs
self.save_input = save_input
...


More some other time.

--Scott David Daniels
scott.daniels@a cm.org

[Frithiof Andreas Jensen]

Re: critique my code, please


"Brian Blais" <bblais@bryant. edu> wrote in message
news:mailman.14 88.1139232769.2 7775.python-list@python.org ...[color=blue]
> Hello,
>
> I am including at the end of this document (is it better as an[/color]
attachment?) some code[color=blue]
> for a small gui dialog. Since I am quite new to this, if anyone has any[/color]
suggestions[color=blue]
> for improvements to the code, bad coding practices, poor gui design,[/color]
etc... I'd love[color=blue]
> to hear it.[/color]

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.