Thanks a lot for the feedback. I have some comments below.
On Mon, Jul 30, 2012 at 10:21 PM, Marcus von Appen <mva@xxxxxxxxxxxx>
just some general things from my "lessons learned"[tm]:
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 .
> 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 .
> I've decided to make use of Python's properties feature  to have more
> control on these attributes, and apply some calculations in their getters
> and setters.
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.
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
You're absolutely right on this one. I'll need to rewrite this part of the class code. Thanks.