Can I get a code review?

Aaron_Leiby · May 13, 2006, 12:34am

I've attached my first real effort using wxPython. I had stalled
until Robin and Noel's book came out, and got through about half the
book before giving in to my desire to start coding. I've written some
python in the past, but mostly just ugly scripts for doing various
permutations of search and replace. So most of this code will look a
lot like the examples in the book, plus I also picked up a couple
tricks from the first chapter of the Python Cookbook. Beyond that, I
have no idea what the "right way" to do a lot of these things are, and
am hoping some people here might be willing to wade through this code
and give me some pointers.

Overall, I'm pretty happy with how it's going (I'm really loving both
Python and wxPython). I'm mostly looking for issues like those
already mentioned: don't pass in pos to PopupMenu, use ListCtrl with
Report/NoHeader flags instead of ListBox, etc.

Please don't take this as a request for free QA support. I think I've
got a pretty good handle on that already, and imagine that would come
across as rather rude.

I guess some context for this code might help. I wanted to make a
simple editor for declaring C++ classes. This will be used to export
class definition headers and also extra data that can be loaded at
runtime to build up meta data for those classes. That data will then
be used to implement features such as automatic serialization,
downcasting, etc.

I've got a fair amount of experience writing code (mostly C++), just
not in Python or wxPython. Any bits of shared wisdom will be most
appreciated.

P.S. Andrea, the HitTest code you posted worked great, although I'm
going to try switching over to a ListCtrl at Robin's suggestion and
see how that works. First, though, I wanted to write up a bunch of
unittests so I don't break too much in the conversion - and as a good
excuse to learn how unittesting works in a gui environment. Some day
I will learn to write my unit tests first.

MetaEd.py (26.7 KB)

CircularReference.py (291 Bytes)

···

On 5/11/06, Andrea Gavana <andrea.gavana@gmail.com> wrote:

I don't think that a newbie that really wants to learn will ever be
considered as spammer if he/she posts some code. I think here there
are a lot of powerful coders that are glad to help newbies as long as
they demonstrate that they are putting a real effort in coding

Rune_Devik · May 13, 2006, 12:59am

Just an remark. If this is one of your first time writing python I’m impressed. The only thing missing (language independent comment) is comments in your code. If you want others to read your code (or you want to understand your own code in 6-12 months) it is highly recommended to use a lot of comments Do not comment each line but comment what each method does, what the class implements and so on. Rock on!

···

On 5/13/06, Aaron Leiby aaron.wxpython@gmail.com wrote:

On 5/11/06, Andrea Gavana andrea.gavana@gmail.com wrote:

I don’t think that a newbie that really wants to learn will ever be
considered as spammer if he/she posts some code. I think here there
are a lot of powerful coders that are glad to help newbies as long as

they demonstrate that they are putting a real effort in coding

I’ve attached my first real effort using wxPython. I had stalled
until Robin and Noel’s book came out, and got through about half the

book before giving in to my desire to start coding. I’ve written some
python in the past, but mostly just ugly scripts for doing various
permutations of search and replace. So most of this code will look a

lot like the examples in the book, plus I also picked up a couple
tricks from the first chapter of the Python Cookbook. Beyond that, I
have no idea what the “right way” to do a lot of these things are, and

am hoping some people here might be willing to wade through this code
and give me some pointers.

Overall, I’m pretty happy with how it’s going (I’m really loving both
Python and wxPython). I’m mostly looking for issues like those

already mentioned: don’t pass in pos to PopupMenu, use ListCtrl with
Report/NoHeader flags instead of ListBox, etc.

Please don’t take this as a request for free QA support. I think I’ve
got a pretty good handle on that already, and imagine that would come

across as rather rude.

I guess some context for this code might help. I wanted to make a
simple editor for declaring C++ classes. This will be used to export
class definition headers and also extra data that can be loaded at

runtime to build up meta data for those classes. That data will then
be used to implement features such as automatic serialization,
downcasting, etc.

I’ve got a fair amount of experience writing code (mostly C++), just

not in Python or wxPython. Any bits of shared wisdom will be most
appreciated.

P.S. Andrea, the HitTest code you posted worked great, although I’m
going to try switching over to a ListCtrl at Robin’s suggestion and

see how that works. First, though, I wanted to write up a bunch of
unittests so I don’t break too much in the conversion - and as a good
excuse to learn how unittesting works in a gui environment. Some day

I will learn to write my unit tests first.

To unsubscribe, e-mail: wxPython-users-unsubscribe@lists.wxwidgets.org

For additional commands, e-mail: wxPython-users-help@lists.wxwidgets.org

–
Rune Devik

http://www.garageinnovation.org

J_Grimmtooth · May 15, 2006, 12:10pm

“Any code you haven’t looked at for six months might have well been written by somebody else”. I don’t know where I picked that up, but it’s totally true

···

On 5/12/06, Rune Devik rune.devik@gmail.com wrote:

The only thing missing (language independent comment) is comments in your code. If you want others to read your code (or you want to understand your own code in 6-12 months)

–
“Ladies and gentlemen, there’s nothing to worry about … but please keep your heads down.” - The Muppet Show

Best,

Jeff

Chris_Barker1 · May 15, 2006, 4:53pm

Just a couple comments:

I suppose it's a taste thing mostly, but I like to bind at the lowest level, so:

self.Bind(wx.EVT_TEXT_ENTER, self.OnNameEnter, self.className)

becomes:

self.className.Bind(wx.EVT_TEXT_ENTER, self.OnNameEnter)

It's a little cleaner, and it works for both Command and non-Command events.

Also, I see a lot of "getters" and "setter". I think it's more pythonic to use just plain attribute access or properties. See:

http://dirtsimple.org/2004/12/python-is-not-java.html

For a commentary that addresses this and other issues.

-Chris

···

--
Christopher Barker, Ph.D.
Oceanographer

NOAA/OR&R/HAZMAT (206) 526-6959 voice
7600 Sand Point Way NE (206) 526-6329 fax
Seattle, WA 98115 (206) 526-6317 main reception

Chris.Barker@noaa.gov

Josiah_Carlson · May 15, 2006, 5:30pm

Just a couple comments:

I suppose it's a taste thing mostly, but I like to bind at the lowest
level, so:

self.Bind(wx.EVT_TEXT_ENTER, self.OnNameEnter, self.className)

becomes:

self.className.Bind(wx.EVT_TEXT_ENTER, self.OnNameEnter)

It's a little cleaner, and it works for both Command and non-Command events.

I agree. I think that whenever possible, users should bind everything
to the control eminating the events.

Also, I see a lot of "getters" and "setter". I think it's more pythonic
to use just plain attribute access or properties. See:

http://dirtsimple.org/2004/12/python-is-not-java.html

wxPython has been around for longer than Python properties. At some
point it may make sense to switch from getters and setters to properties,
though I think that some people would be confused for something like...

self.textctrl.value = 10

To raise a ValueError.

- Josiah

···

Christopher Barker <Chris.Barker@noaa.gov> wrote:

Peter_Decker · May 15, 2006, 5:37pm

Dabo's ui module is based on wxPython, and uses properties. It
produces much cleaner code. Which do you like better:

self.txtbox.Value += 1

-or-

self.txtbox.SetValue(self.txtbox.GetValue() + 1)

Having worked almost exclusively with the Dabo.ui for the last few
months, I find my old raw wxPython code ugly and cumbersome.

···

On 5/15/06, Josiah Carlson <jcarlson@uci.edu> wrote:

> Also, I see a lot of "getters" and "setter". I think it's more pythonic
> to use just plain attribute access or properties. See:
>
> http://dirtsimple.org/2004/12/python-is-not-java.html

wxPython has been around for longer than Python properties. At some
point it may make sense to switch from getters and setters to properties,
though I think that some people would be confused for something like...

self.textctrl.value = 10

To raise a ValueError.

--

# p.d.

Nitro · May 15, 2006, 5:58pm

I wonder how hard it is to get this feature into the wxPython core. AFAIK during the wxPython build xml files are scanned to generate %renames to fix the wx prefix for all classes. Now one could probably extend this scanning and let the scanner see if there's a Set*/Get* method. If yes, then it could automatically emit code like here ( http://svn.berlios.de/svnroot/repos/pyogre/trunk/pyogre/cegui/cegui_attributes.i ) for example to tell swig to generate properties in addition to the Get/Set functions. This would take just a few hours to implement I guess.
This probably won't work for all classes (methods don't follow the Get/Set naming convention for example), but it would work in 70% I guess. If you'd also incorporate the cases where only a Get method is provided you could boost this by another 10 or 15% maybe.

What do you think?

-Matthias

···

Am Mon, 15 May 2006 19:37:53 +0200 hat Peter Decker <pydecker@gmail.com> geschrieben:

On 5/15/06, Josiah Carlson <jcarlson@uci.edu> wrote:

> Also, I see a lot of "getters" and "setter". I think it's more pythonic
> to use just plain attribute access or properties. See:
>
> http://dirtsimple.org/2004/12/python-is-not-java.html

wxPython has been around for longer than Python properties. At some
point it may make sense to switch from getters and setters to properties,
though I think that some people would be confused for something like...

self.textctrl.value = 10

To raise a ValueError.

Dabo's ui module is based on wxPython, and uses properties. It
produces much cleaner code. Which do you like better:

self.txtbox.Value += 1

-or-

self.txtbox.SetValue(self.txtbox.GetValue() + 1)

Having worked almost exclusively with the Dabo.ui for the last few
months, I find my old raw wxPython code ugly and cumbersome.

Chris_Barker1 · May 15, 2006, 6:29pm

Josiah Carlson wrote:

Also, I see a lot of "getters" and "setter". I think it's more pythonic to use just plain attribute access or properties. See:

http://dirtsimple.org/2004/12/python-is-not-java.html

wxPython has been around for longer than Python properties. At some
point it may make sense to switch from getters and setters to properties,
though I think that some people would be confused for something like...

self.textctrl.value = 10

Well, wxPython itself mirrors the C++ API, so getters and setters are inevitable.

I was referring to the users own code, and when I've used libs that use properties, it hasn't confused me. And while properties haven't been around all that long, you could always achieve the same thing by overriding __setattr__, and there is a longish history of that, at least for the Numeric module.

What's nice about properties is that you don't' have to write getters and setters "just in case" you might need to do something upon getting or setting in the future.

It is a style, thing, however.

-Chris

···

Christopher Barker <Chris.Barker@noaa.gov> wrote:

--
Christopher Barker, Ph.D.
Oceanographer

NOAA/OR&R/HAZMAT (206) 526-6959 voice
7600 Sand Point Way NE (206) 526-6329 fax
Seattle, WA 98115 (206) 526-6317 main reception

Chris.Barker@noaa.gov

Josiah_Carlson · May 15, 2006, 6:29pm

I agree that in the vast majority of use-cases, properties would make
for more readable code. However, note that in my reply I merely
commented about what is observed to be an assignment raising a
ValueError - something that doesn't generally happen with Python, and
that such behavior could confuse users of the language.

- Josiah

···

"Peter Decker" <pydecker@gmail.com> wrote:

On 5/15/06, Josiah Carlson <jcarlson@uci.edu> wrote:
> > Also, I see a lot of "getters" and "setter". I think it's more pythonic
> > to use just plain attribute access or properties. See:
> >
> > http://dirtsimple.org/2004/12/python-is-not-java.html
>
> wxPython has been around for longer than Python properties. At some
> point it may make sense to switch from getters and setters to properties,
> though I think that some people would be confused for something like...
>
> self.textctrl.value = 10
>
> To raise a ValueError.

Dabo's ui module is based on wxPython, and uses properties. It
produces much cleaner code. Which do you like better:

self.txtbox.Value += 1

-or-

self.txtbox.SetValue(self.txtbox.GetValue() + 1)

Having worked almost exclusively with the Dabo.ui for the last few
months, I find my old raw wxPython code ugly and cumbersome.

Nitro · May 15, 2006, 6:39pm

I agree that in the vast majority of use-cases, properties would make
for more readable code. However, note that in my reply I merely
commented about what is observed to be an assignment raising a
ValueError - something that doesn't generally happen with Python, and
that such behavior could confuse users of the language.

These problems could be fixed by telling SWIG to use some typemaps when generating the properties as I explained in my other post. For example whenever the C++ side expects a string and the python user puts a number in there, it could get converted automatically.

-Matthias

Nitro · May 15, 2006, 6:50pm

Or better with something similar to the OOR magic, namely if an int or float was set to the property, then an int or float should also be returned instead of a string.

-Matthias

···

Am Mon, 15 May 2006 20:39:20 +0200 hat Nitro <nitro@dr-code.org> geschrieben:

I agree that in the vast majority of use-cases, properties would make
for more readable code. However, note that in my reply I merely
commented about what is observed to be an assignment raising a
ValueError - something that doesn't generally happen with Python, and
that such behavior could confuse users of the language.

These problems could be fixed by telling SWIG to use some typemaps when generating the properties as I explained in my other post. For example whenever the C++ side expects a string and the python user puts a number in there, it could get converted automatically.

Aaron_Leiby · May 16, 2006, 7:34am

Thanks everybody for the great feedback!

Just a couple comments:

I suppose it's a taste thing mostly, but I like to bind at the lowest
level, so:

Good call, I agree it's more readable after making the changes. I
originally started with MenuItems (which don't support Bind?) and kind
of followed example from there.

Also, I see a lot of "getters" and "setter". I think it's more pythonic
to use just plain attribute access or properties. See:

http://dirtsimple.org/2004/12/python-is-not-java.html

Neat. I knew C# had that type of support, but hadn't seen it in
Python. I had dabbled a bit in jython, and saw they wrapped things up
similarly (using java beans?) and was kind of disappointed to find all
the get/set interfaces when I first started looking at wxPython.

To address some of my specific situations...

I had wrapped the class list's self.selected access in GetSelected so
I could pass it in as a function callback to the class selector. Is
there a way to do that without using a function?

Each of the model classes have a GetData which I'm using to strip out
the listener InstanceMethods so I can pickle them. I could see
wrapping these up in properties, but then I'd be tempted to change my
Save code to the following:

    def Save(self, filename):
        # break object references
        for d in self.data:
            d.parent = self.ToIndex(d.parent)
            for p in d.props:
                p.inner = self.ToIndex(p.inner)
        f = open(filename, 'w')
        try: cPickle.dump(self.data, f)
        finally: f.close()

Which wouldn't work very well since self.data would be dynamically
constructed each time it's accessed. So I'd clean up all the
references in the first pass, then lose all the changes when I called
self.data again to pickle it.

I could push the object reference breaking into the accessor itself,
but there are other situations where I'd like to leave the references
intact. Plus it's currently nice and symmetrical with the Load code
which wouldn't refactor as nicely.

My original plan was to store the data in XML, but after reading that
rant, I may need to give second thought to the matter. I initially
used pickle just to get some quick persistence, but I'm finding it
doesn't version very well - I refactored some code and already had to
hand edit my default.meta file.

The majority of the setters are used to trigger an update for the
listeners. These wrapped up pretty nicely using properties, though I
agree that it seems a bit weird when setting a property throws an
exception.

The SetModel stuff was a little less clear for me since some of them
take extra parameters. This is probably just a sign of bad underlying
design, but it's not yet clear to me the best way to clean it up. I
suppose I could always use tuples in the assignment, but that seems a
bit much.

In any case, thanks again for the feedback. You guys are great!

···

On 5/15/06, Christopher Barker <Chris.Barker@noaa.gov> wrote:

Robin · May 17, 2006, 7:10am

Aaron Leiby wrote:

Thanks everybody for the great feedback!

Just a couple comments:

I suppose it's a taste thing mostly, but I like to bind at the lowest
level, so:

Good call, I agree it's more readable after making the changes. I
originally started with MenuItems (which don't support Bind?)

You can use the menu item as the source parameter of a Bind called on the frame. IOW:

frame.Bind(wx.EVT_MENU, self.OnDoSomething, item)

···

On 5/15/06, Christopher Barker <Chris.Barker@noaa.gov> wrote:

--
Robin Dunn
Software Craftsman
http://wxPython.org Java give you jitters? Relax with wxPython!