cleaner way to write this try/except statement?

[John Salerno]

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

Thanks.

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

import wx
class NumbersValidator(wx.PyValidator):

def __init__(self):
wx.PyValidator.__init__(self)

def Clone(self):
return NumbersValidator()

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
wx.MessageBox('Enter a valid time.', 'Invalid time

entered', wx.OK | wx.ICON_ERROR)
return False
else:
return True
except ValueError, error:
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

def TransferToWindow(self):
return True

def TransferFromWindow(self):
return True
class Timer(wx.Frame):

def __init__(self):
wx.Frame.__init__(self, None, wx.ID_ANY, 'Timer',
style=wx.DEFAULT_FRAME_STYLE ^ wx.RESIZE_BORDER
^ wx.MAXIMIZE_BOX)
self.panel = wx.Panel(self)

mainSizer = wx.BoxSizer(wx.VERTICAL)
inputSizer = wx.BoxSizer(wx.HORIZONTAL)

self.progress = wx.Gauge(self.panel, wx.ID_ANY, 100, size=(300,
20))
self.status = wx.StaticText(self.panel, wx.ID_ANY, 'Enter a time.')
prompt = wx.StaticText(self.panel, wx.ID_ANY, 'Time to wait:')
self.input = wx.TextCtrl(self.panel, wx.ID_ANY, size=(20, 20),
validator=NumbersValidator())
self.start = wx.Button(self.panel, wx.ID_ANY, 'Start')
self.reset = wx.Button(self.panel, wx.ID_ANY, 'Reset')
self.reset.Disable()
self.sound = wx.Sound(r'C:\Windows\Media\notify.wav')
self.timer = wx.Timer(self)

mainSizer.Add(self.progress, flag=wx.ALIGN_CENTER | wx.ALL ^
wx.BOTTOM,
border=10)
mainSizer.Add(self.status, flag=wx.ALIGN_CENTER | wx.ALL,
border=10)
mainSizer.Add(inputSizer, flag=wx.ALIGN_CENTER | wx.BOTTOM,
border=10)
inputSizer.Add(prompt, flag=wx.ALIGN_CENTER)
inputSizer.Add(self.input, flag=wx.ALIGN_CENTER | wx.LEFT |
wx.RIGHT,
border=5)
inputSizer.Add(self.start, flag=wx.ALIGN_CENTER)
inputSizer.Add(self.reset, flag=wx.ALIGN_CENTER)

self.Bind(wx.EVT_TEXT_ENTER, self.OnStart, self.input)
self.Bind(wx.EVT_BUTTON, self.OnStart, self.start)
self.Bind(wx.EVT_TIMER, self.OnTimer, self.timer)
self.Bind(wx.EVT_BUTTON, self.OnReset, self.reset)

self.panel.SetSizer(mainSizer)
mainSizer.Fit(self)

def OnStart(self, event):
if self.panel.Validate():
self.time = int(self.input.GetValue())
self.minutes_passed = 1
self.minutes_remaining = self.time - 1
self.start.Disable()
self.input.Disable()
self.reset.Enable()
self.status.SetLabel('%s minute(s) remaining.' % self.time)
self.timer.Start(1000)

def OnReset(self, event):
if self.timer.IsRunning():
self.timer.Stop()
self.input.Clear()
self.input.Enable()
self.start.Enable()
self.reset.Disable()
self.status.SetLabel('Enter a new time.')
self.progress.SetValue(0)
self.minutes_passed = 1

def OnTimer(self, event):
if self.minutes_remaining != 0:
self.progress.SetValue(self.minutes_passed * 100 / self.time)
self.status.SetLabel('%s minute(s) remaining.' %
self.minutes_remaining)
self.minutes_passed += 1
self.minutes_remaining -= 1
else:
self.timer.Stop()
self.progress.SetValue(self.minutes_passed * 100 / self.time)
self.status.SetLabel('%s minute(s) have elapsed.' % self.time)
wx.Sound.Play(self.sound)
class MyApp(wx.App):

def OnInit(self):
frame = Timer()
self.SetTopWindow(frame)
frame.Show()
return True
if __name__ == '__main__':
app = MyApp(False)
app.MainLoop()

[John Salerno]

John Salerno wrote:

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

I cleaned it up a little and did this, but of course this doesn't work.
Is this the wrong way to create a function (not method) within a class?

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
error_message()
return False
else:
return True
except ValueError:
error_message()
return False

@staticmethod
def error_message():
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)

[Simon Forman]

John Salerno wrote:

John Salerno wrote:

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

I cleaned it up a little and did this, but of course this doesn't work.
Is this the wrong way to create a function (not method) within a class?

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
error_message()
return False
else:
return True
except ValueError:
error_message()
return False

@staticmethod
def error_message():
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)

Your indentation looks off. Your error_message() function should be at
the same indentation as the rest of the body of the Validate() method
(i.e. same as the try statement, "text_ctrl = ..." statements.)

If that's not an artifact of posting it here then you'll need to
correct that.

Also, there's no need to declare the function a staticmethod, since it
isn't.

Other than that it looks ok to me, but I might have missed something.

Peace,
~Simon

[Simon Forman]

Simon Forman wrote:

John Salerno wrote:

John Salerno wrote:

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

I cleaned it up a little and did this, but of course this doesn't work.
Is this the wrong way to create a function (not method) within a class?

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
error_message()
return False
else:
return True
except ValueError:
error_message()
return False

@staticmethod
def error_message():
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)

Your indentation looks off. Your error_message() function should be at
the same indentation as the rest of the body of the Validate() method
(i.e. same as the try statement, "text_ctrl = ..." statements.)

If that's not an artifact of posting it here then you'll need to
correct that.

Also, there's no need to declare the function a staticmethod, since it
isn't.

Other than that it looks ok to me, but I might have missed something.

Maybe I did miss something: Are you actually trying to make a static
method? Say, to be able to call it elsewhere than just the Validate()
method?

In that case, your indentation is still (apparently) off, but the
error_message() method (a staticmethod is still a method I think, but I
could be wrong) should be at the same indentation level as any other
method, and you have to prepend the instance object ('self.' if you're
calling it from inside another method) to it when you call it.

class A(object):
def meth(self, arg):
self.func(arg)

@staticmethod
def func(arg):
print arg

a = A()

a.func("hi")
a.meth("world.")
# prints
hi
world.

HTH,
~Simon

[infidel]

Here's how I would do it:

def Validate(self, parent):
try:
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()
if not text: raise ValueError
if int(text) <= 0: raise ValueError
return True
except ValueError, error:
wx.MessageBox('Enter a valid time.', 'Invalid time
entered', wx.OK | wx.ICON_ERROR)
return False

[Boris Borcic]

John Salerno wrote:

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

Thanks.

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

import wx
class NumbersValidator(wx.PyValidator):

def __init__(self):
wx.PyValidator.__init__(self)

def Clone(self):
return NumbersValidator()

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
wx.MessageBox('Enter a valid time.', 'Invalid time
entered', wx.OK | wx.ICON_ERROR)
return False
else:
return True
except ValueError, error:
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

well, assuming you are unsatisfied with the above, you could try to assert the
validation condition and catch together all failures, eg :

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()
try :
assert int(text)>0
return True
except (ValueError,AssertionError) :
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

hth, BB

[Simon Forman]

John Salerno wrote:

John Salerno wrote:

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

I cleaned it up a little and did this, but of course this doesn't work.
Is this the wrong way to create a function (not method) within a class?

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
error_message()
return False
else:
return True
except ValueError:
error_message()
return False

@staticmethod
def error_message():
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)

If you're not really interested in methods, static or otherwise, and
actually just want a cleaner Validate() method, I would code it as
follows (note that calling int() on an empty string will raise
ValueError):
def Validate(self, parent):
text = self.GetWindow().GetValue()

try:
T = int(text)
except ValueError:
result = False
else:
result = T 0

if not result:
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)

return result

Peace,
~Simon

[Simon Forman]

Boris Borcic wrote:

John Salerno wrote:

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

Thanks.

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

import wx
class NumbersValidator(wx.PyValidator):

def __init__(self):
wx.PyValidator.__init__(self)

def Clone(self):
return NumbersValidator()

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
wx.MessageBox('Enter a valid time.', 'Invalid time
entered', wx.OK | wx.ICON_ERROR)
return False
else:
return True
except ValueError, error:
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

well, assuming you are unsatisfied with the above, you could try to assert the
validation condition and catch together all failures, eg :

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()
try :
assert int(text)>0
return True
except (ValueError,AssertionError) :
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

hth, BB

Assertion statements "go away" when you run python with the '-O' or
'-OO' options. They're only meant for debugging and shouldn't be used
as part of your actual program logic.

You run the risk of introducing hard-to-find bugs if you use them like
this and somebody, somewhere, sometime runs your code in "optimized"
mode.

Peace,
~Simon

[Peter Otten]

John Salerno wrote:

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

Thanks.

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

import wx
class NumbersValidator(wx.PyValidator):

def __init__(self):
wx.PyValidator.__init__(self)

def Clone(self):
return NumbersValidator()

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
wx.MessageBox('Enter a valid time.', 'Invalid time

entered', wx.OK | wx.ICON_ERROR)
return False
else:
return True
except ValueError, error:
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

def TransferToWindow(self):
return True

def TransferFromWindow(self):
return True

Here's how I might do it:

def is_positive_int_str(s):
try:
value = int(s)
except ValueError:
return False
return value 0

class PositiveIntegerValidator(wx.PyValidator):
# ...
def Validate(self, parent):
text = self.GetWindow().GetValue()
if is_positive_int_str(text):
return True
wx.MessageBox(...)
return False

Two usability notes:
- "Invalid time entered" is a confusing message when you actually want a
positive integer.
- Pop-up messages are a major annoyance. An alternative would be to change
the text color of the control and show a hint as to what input you expect
in a status bar.

Peter

[John Salerno]

Peter Otten wrote:

John Salerno wrote:

>The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

Thanks.

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

import wx
class NumbersValidator(wx.PyValidator):

def __init__(self):
wx.PyValidator.__init__(self)

def Clone(self):
return NumbersValidator()

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
wx.MessageBox('Enter a valid time.', 'Invalid time

entered', wx.OK | wx.ICON_ERROR)
return False
else:
return True
except ValueError, error:
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

def TransferToWindow(self):
return True

def TransferFromWindow(self):
return True

Here's how I might do it:

def is_positive_int_str(s):
try:
value = int(s)
except ValueError:
return False
return value 0

class PositiveIntegerValidator(wx.PyValidator):
# ...
def Validate(self, parent):
text = self.GetWindow().GetValue()
if is_positive_int_str(text):
return True
wx.MessageBox(...)
return False

Two usability notes:
- "Invalid time entered" is a confusing message when you actually want a
positive integer.
- Pop-up messages are a major annoyance. An alternative would be to change
the text color of the control and show a hint as to what input you expect
in a status bar.

Peter

Thanks for all the help guys! I'm going to rework it a little and see
what happens. Luckily, I'm also reading the chapter on exceptions in
Python in a Nutshell, so maybe that will help solidify my understanding
of how to construct them in this situation.

[John Salerno]

Simon Forman wrote:

Maybe I did miss something: Are you actually trying to make a static
method? Say, to be able to call it elsewhere than just the Validate()
method?

I don't know if a static method is the best way to do it, but what I was
trying to do was make a regular function within the validator class, so
that I could call it whenever I needed to show that error message. I
suppose I could just use a regular method to do it, too, but it seems
more like a utility function than a method that requires 'self'.

My identation seems correct to me (and I get no errors about it), but
it's still not working the way I have it. Maybe I shouldn't mess with
decorators right now anyway...

[John Salerno]

John Salerno wrote:

The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

Ok, I came up with this. It looks much nicer, I think, though I'm still
unsure if it's good to have three separate return statements:

def Validate(self, parent):
text = self.GetWindow().GetValue()

try:
if int(text) != 0:
return True
else:
self.error_message()
return False
except ValueError:
self.error_message()
return False

But instead of a pop-up box for the error, I've decided to take Peter's
advice and change the status text instead (plus color the input box).
But here's an ultra OO newbie question: how do I refer to my frame's
children from within the validator class? :)

[Roy Smith]

John Salerno <jo******@NOSPAMgmail.comwrote:

try:
if int(text) != 0:
return True
else:
self.error_message()
return False
except ValueError:
self.error_message()
return False

One possible refactoring would be:

try:
if int(text) != 0:
return True
except:
pass
self.error_message()
return False

It's two less lines of code, one less return, and eliminates the
duplicate call to error_message(). On the other hand, the "except:
pass" clause is unusual and would make me (as a reader of the code)
stop and think what was going on. Pick your poison.

If you want to be truly cryptic and obfuscatory, there's always:

try:
1 / int(text)
return True
except:
return False

[Boris Borcic]

Simon Forman wrote:

Boris Borcic wrote:

>John Salerno wrote:

>>The code to look at is the try statement in the NumbersValidator class,
just a few lines down. Is this a clean way to write it? i.e. is it okay
to have all those return statements? Is this a good use of try? Etc.

Thanks.

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

import wx
class NumbersValidator(wx.PyValidator):

def __init__(self):
wx.PyValidator.__init__(self)

def Clone(self):
return NumbersValidator()

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()

try:
if not text or int(text) <= 0:
wx.MessageBox('Enter a valid time.', 'Invalid time
entered', wx.OK | wx.ICON_ERROR)
return False
else:
return True
except ValueError, error:
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

well, assuming you are unsatisfied with the above, you could try to assert the
validation condition and catch together all failures, eg :

def Validate(self, parent):
text_ctrl = self.GetWindow()
text = text_ctrl.GetValue()
try :
assert int(text)>0
return True
except (ValueError,AssertionError) :
wx.MessageBox('Enter a valid time.', 'Invalid time entered',
wx.OK | wx.ICON_ERROR)
return False

hth, BB

Assertion statements "go away" when you run python with the '-O' or
'-OO' options.

Makes me wonder how many use those options. I never do, this explains that.

They're only meant for debugging and shouldn't be used
as part of your actual program logic.

That's too bad, and bound to evolve if the logic object space of Pypy gains users.

You run the risk of introducing hard-to-find bugs if you use them like
this and somebody, somewhere, sometime runs your code in "optimized"
mode.

Well, I wouldn't formulate it quite like that, but, as you say,...

Peace,
~Simon

Well, in that case let's just define

def fail(exc) : raise exc

and replace

assert int(text)>0

by

int(text)>0 or fail(ValueError)

cheers, BB
--
"On naît tous les mètres du même monde"