Hey Marcus, Thanks a lot for the feedback. I have some comments below.
On Mon, Jul 30, 2012 at 10:21 PM, Marcus von Appen <[email protected]> wrote: > Hi, > > On, Mon Jul 30, 2012, Sagie Maoz wrote: > > > Hi guys, > > > > I would like to have your opinion on some decisions I made when writing a > > new Sprite class, as part of my Summer of Code project [1]. > > As part of this work. I wrote several new features related to the core > > Sprite class, which can be set and manipulated through new attributes of > > the class. > > You can read about the new attributes and my implementation on my blog > [2]. > > > > I've decided to make use of Python's properties feature [3] to have more > > control on these attributes, and apply some calculations in their getters > > and setters. > > just some general things from my "lessons learned"[tm]: > > use decorators, if possible, in favour of property() - this makes the > code more readable, since it's clear that foo is a property, without > scrolling X hundred lines down in the code: > I started out doing that, but I didn't like how the notation is inconsistent (@property vs. @foo.setter). I see your point about code clarity, though. If no one objects, I'll consider reverting to decorators as you suggested. > > @property > def foo(self): > ... > > @foo.setter > def _setfoo(self, value): > ... > > Avoid useless property encapsulations as in Java/C++/C#/..., as you did > for the "visible" one in your code. You do not do any internal magic, but > just assign it, why would you want a property for that? > Generally I agree, but this was done so that the 'visible' setter could be wrapped with the _visual_set wrapper (that fires a 'visual attribute changed' event). Come to think about that, this was also part of the reason I switched to using the property() notation. Do you have a suggestion on how to implement such events in a clearer manner? > > Note that properties eat up (a few) cycles, since they defer to a method > to be called, etc. If you do not do complex checks, avoid properties. > > Directly code related: > > Your _get_image() code changes internals of the Sprite at run-time - it > is a read operation that CHANGES stuff - do not do that, it's bad for > everyone, one would not usually expect it and it's not even noted in the > doc > strings. > You're absolutely right on this one. I'll need to rewrite this part of the class code. Thanks. > > Cheers > Marcus > -- Your friend in time, Sagie Maoz [email protected] // +1 (347) 556.5044 // +972 (52) 834-3339 http://sagie.maoz.info/ http://n0nick.net/ /* simba says roar! */
