cleaner way to write this try/except statement?

J

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

John Salerno

John said:
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.
Click to expand...

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

Simon Forman

John said:
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)
Click to expand...

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
 
S

Simon Forman

Simon said:
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.
Click to expand...


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
 
I

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
 
B

Boris Borcic

John said:
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
Click to expand...

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
 
S

Simon Forman

John said:
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)
Click to expand...

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
 
S

Simon Forman

Boris said:
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
Click to expand...

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
 
P

Peter Otten

John said:
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
Click to expand...

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
 
J

John Salerno

Peter said:
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
Click to expand...

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.
 
J

John Salerno

Simon said:
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?
Click to expand...

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...
 
J

John Salerno

John said:
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.
Click to expand...

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? :)
 
R

Roy Smith

John Salerno said:
try:
if int(text) != 0:
return True
else:
self.error_message()
return False
except ValueError:
self.error_message()
return False
Click to expand...

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
 
B

Boris Borcic

Simon said:
Assertion statements "go away" when you run python with the '-O' or
'-OO' options.
Click to expand...

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.
Click to expand...

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.
Click to expand...

Well, I wouldn't formulate it quite like that, but, as you say,...
Peace,
~Simon
Click to expand...

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
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

how best to check a value? (if/else or try/except?) 4
what's wrong with this if statement? 2
wxpython: Timer object not working, or just me? 3
wxPython layout problem 1
[wxPython] How to change deafult tab traversing (radiobuttons & panel) 1
Best way to handle exceptions with try/finally 12
wxpython grid GRIDTABLE_NOTIFY_ROWS_DELETED 1
wxpython: can't even create a Panel 3

Members online

Total: 31 (members: 2, guests: 29)
Robots: 93

Forum statistics

Threads
473,768
Messages
2,569,574
Members
45,048
Latest member
verona

Latest Threads

Top