cleaner way to write this?

[John Salerno]

Hi guys. I'm looking for a nicer, more compact way of writing this code.
It doesn't have to be anything fancy, just something without the
duplication and preferably only one return statement.

def create_db_name(self):
dlg = wx.TextEntryDialog(self.frame, 'Enter a database name:',
'Create New Database')
if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
dlg.Destroy()
return db_name
else:
dlg.Destroy()
return

One problem is that if "Cancel" is pressed, I can't return anything.
Another problem is that the dialog must be destroyed, so that has to
come before any return statements.

Thanks.

 

[Farshid Lashkari]

Hi guys. I'm looking for a nicer, more compact way of writing this code.
It doesn't have to be anything fancy, just something without the
duplication and preferably only one return statement.

def create_db_name(self):
dlg = wx.TextEntryDialog(self.frame, 'Enter a database name:',
'Create New Database')
if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
dlg.Destroy()
return db_name
else:
dlg.Destroy()
return

One problem is that if "Cancel" is pressed, I can't return anything.
Another problem is that the dialog must be destroyed, so that has to
come before any return statements.

Thanks.

This should work:

def create_db_name(self):
dlg = wx.TextEntryDialog(self.frame, 'Enter a database name:',
'Create New Database')
db_name = None
if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
dlg.Destroy()
return db_name


-Farshid

 

[Paul Rubin]

if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
dlg.Destroy()
return db_name
else:
dlg.Destroy()
return

I like

if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
else:
db_name = None
dlg.Destroy()
return db_name

better than

db_name = None
if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
dlg.Destroy()
return db_name

but I suppose it's a matter of preference.

 

[John Salerno]

I like

if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
else:
db_name = None
dlg.Destroy()
return db_name

better than

db_name = None
if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
dlg.Destroy()
return db_name

Thanks guys! I didn't think about setting db_name to None in that way!

 

[Farshid Lashkari]

I like

if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
else:
db_name = None
dlg.Destroy()
return db_name

better than

db_name = None
if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
dlg.Destroy()
return db_name

but I suppose it's a matter of preference.

Yeah, I think the second way is usually used by people who are more
accustomed to programming in C, since they need to initialize variables.
Your way is probably more Pythonic though.

-Farshid

 

[John Salerno]

Hi guys. I'm looking for a nicer, more compact way of writing this code.

Ok, one more thing. Let's assume I'm using this (or the other):

def create_db_name(self):
dlg = wx.TextEntryDialog(self.frame, 'Enter a database name:',
'Create New Database')
if dlg.ShowModal() == wx.ID_OK:
db_name = dlg.GetValue()
else:
db_name = None
dlg.Destroy()
return db_name

Now, one thing I want to prevent is assigning db_name the value of the
text when OK is pressed if the text is an empty string. So I was
thinking I could do this:

if dlg.ShowModal() == wx.ID_OK and dlg.GetValue()

I think 'dlg' would exist by the time Python evaluates the second half
of the expression. But this would involve using dlg.GetValue() again
within the if statement, which seems redundant.

But I'm also trying to avoid a lot of little if tests all over the
place, and I definitely don't want to nest them if possible.

Of course, just using the above line isn't good enough, because this
still assigns db_name to None and returns it. Instead, I'll need an
error dialog box to pop up. This will involve a wxPython Validator class
probably (although I suppose this is simple enough not to), but before I
get to all that, I just need some advice for how to structure the check
of the empty string.

Thanks again.

 

[Paul Rubin]

I just need some advice for how to structure
the check of the empty string.

How about

return db_name or None

since the empty string taken as a boolean is False.

 

[John Salerno]

How about

return db_name or None

since the empty string taken as a boolean is False.

But if the user doesn't enter any text, I don't want the method to
return at all (even None). I need an error dialog to pop up and tell him
to enter a value (or press Cancel).

 

[Paul Rubin]

But if the user doesn't enter any text, I don't want the method to
return at all (even None). I need an error dialog to pop up and tell
him to enter a value (or press Cancel).

Oh, I see. You really want something like repeat...while, which Python
doesn't have. Anyway I start to prefer something like (untested):

def create_db_name(self):
try:
while True:
dlg = wx.TextEntryDialog(self.frame, 'Enter a database name:',
'Create New Database')
if dlg.ShowModal() != wx.ID_OK:
return None
db_name = dlg.GetValue()
if db_name:
return db_name
pop_up_error_dialog("please enter a value or press cancel")
finally:
dlg.Destroy()

If the logic were more complex you could also create an internal
exception class to deal with the user hitting cancel.

 

[John Salerno]

Oh, I see. You really want something like repeat...while, which Python
doesn't have. Anyway I start to prefer something like (untested):

def create_db_name(self):
try:
while True:
dlg = wx.TextEntryDialog(self.frame, 'Enter a database name:',
'Create New Database')
if dlg.ShowModal() != wx.ID_OK:
return None
db_name = dlg.GetValue()
if db_name:
return db_name
pop_up_error_dialog("please enter a value or press cancel")
finally:
dlg.Destroy()

Interesting. Some variation of this might suit me, but I think one other
problem I'm having is that the ShowModal() method returns whenever a
button is pressed, so even if OK is pressed with an empty string in the
box, the dialog disappears and returns an "OK" value. I'll have to check
in the wxpython group on how to handle this one, I think.

 

[Paul Rubin]

Interesting. Some variation of this might suit me, but I think one
other problem I'm having is that the ShowModal() method returns
whenever a button is pressed, so even if OK is pressed with an empty
string in the box, the dialog disappears and returns an "OK" value.

Right. The above handles that case, I believe. If the user clicks
with an empty string, showModal() returns wx.ID_OK, GetValue() returns
the empty string, db_name gets set to that empty string, db_name gets
tested (maybe I should have made the comparison with the empty string
explicit), the error dialog pops up since db_name is empty, and the
loop repeats.

 

[John Salerno]

Right. The above handles that case, I believe.

Oh, you're right! It's amazing how this went from a simple, two-line
method to a big mess of error-checking.

 

[Neil Cerutti]

Oh, you're right! It's amazing how this went from a simple,
two-line method to a big mess of error-checking.

From Kernighan & Pike, _The Practice of Programming_ (ISBN
0-201-61586-X):

This completes our C version [of a csv reading library]. It
handles arbitrarily large inputs and does something sensible
even with perverse data. The price is that it is more than four
times as long as the first prototype and some of the code is
intricate. Such expansion of size and complexity is a typical
result of moving from prototype to production.

(It's a useful book, especially for a hobbyist programmer. Though
there's not much Python code in it, pretty much all the advice
inside is applicable to writing Python code.)

 

[John Salerno]

This completes our C version [of a csv reading library]. It
handles arbitrarily large inputs and does something sensible
even with perverse data. The price is that it is more than four
times as long as the first prototype and some of the code is
intricate. Such expansion of size and complexity is a typical
result of moving from prototype to production.

LOL. Guess I'm doing things right, then?

 

[John Salerno]

def create_db_name(self):
try:
while True:
dlg = wx.TextEntryDialog(self.frame, 'Enter a database name:',
'Create New Database')
if dlg.ShowModal() != wx.ID_OK:
return None
db_name = dlg.GetValue()
if db_name:
return db_name
pop_up_error_dialog("please enter a value or press cancel")
finally:
dlg.Destroy()

Interesting idea to use try/finally to ensure that dlg.Destroy() runs
even with a return earlier in the method. Would this be considered
appropriate use though, or would it be misusing try/finally? I thought
the point of a try block was for when you anticipated exceptions, but
given that try/except and try/finally used to be separate blocks,
perhaps this was the original intent of a try/finally block? (i.e.
nothing to do with exceptions?)

 

[Paul Rubin]

Interesting idea to use try/finally to ensure that dlg.Destroy() runs
even with a return earlier in the method. Would this be considered
appropriate use though, or would it be misusing try/finally? I thought
the point of a try block was for when you anticipated exceptions,

I think it's ok to treat the return statement as sort of an exception.
You could create an actual exception and raise it instead, but in such
a small function that clutters up the code unnecessarily. At most I'd
add a comment near the return statement, mentioning that the finally:
clause will clean up the dialog.

but given that try/except and try/finally used to be separate
blocks,

That old separation was just an artifact of how the parser was
originally written, I believe. It was always considered an annoyance
and it finally got fixed.

 

[Fredrik Lundh]

That old separation was just an artifact of how the parser was
originally written, I believe.

$ more Misc/HISTORY

New features in 0.9.6:
- stricter try stmt syntax: cannot mix except and finally clauses
on 1 try

</F>

 

[Paul Rubin]

Interesting idea to use try/finally to ensure that dlg.Destroy() runs
even with a return earlier in the method.

Note that the code is wrong, the dialog should either be created
outside the while loop, or destroyed inside it. I don't know wx so
I'm not sure which of those is correct. But the version I posted
potentially creates multiple dialogs and destroys only one of them.

 

[John Salerno]

Note that the code is wrong, the dialog should either be created
outside the while loop, or destroyed inside it. I don't know wx so
I'm not sure which of those is correct. But the version I posted
potentially creates multiple dialogs and destroys only one of them.

oh you're right...seems like it will happen if the error occurs....

 

[Paul Rubin]

John, please re-read the FineManual(tm). None is the default return
value of a function - even if there's no return statement.

Correct, but if the user doesn't enter text, the function is supposed
to loop and prompt for text again, instead of returning.