Hi all,
Following my original thread, and given the feedback I received (here and mainly from #pygame's TheSheep), I have decided to refactor the attribute getters and setters in my code to accommodate an easier "nouns-verbs" convention, that is, nouns for getters and verbs for setters.
With this approach, I was able to avoid unnecessary wrapping of simple attribute calls, while maintaining the same functionality.
I also removed the (admittedly badly designed) logic that required changing the class's state as part of the `image` getter.
I'm using a Python decorator `@visual_change` to mark methods that visually alter the sprite, and should trigger some general code and also allow an optional callback function. This is used, for example, to mark sprites dirty, and in my `AggregatedSprite` class, to propagate the changes to all child sprites.
The new refactored code is here:
I would love to get feedback on the code style idea and any thoughts you might have on my implementation.
Thanks, and good day! :)
On Mon, Jul 30, 2012 at 10:27 PM, Sagie Maoz
<sagie@xxxxxxxxx> wrote:
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
<mva@xxxxxxxxxxxx> 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
--