parent-child object design question

[Steven D'Aprano]

On Wed, 31 Jan 2007 20:15:44 +1100, Ben Finney wrote:

> "Steven D'Aprano" <steve at REMOVEME.cybersource.com.au> writes:
> 
>> >         def _accumulate_properties(self, properties):
>> >             self.properties = []
>>
>> Probably better to put that in the __init__ method, otherwise if
>> somebody runs instance._accumulate_properties(...) again, it will
>> have the side-effect of throwing away whatever was already in
>> instance.properties.
> 
> That's the point of naming it with a leading underscore. You've
> already explained that this is convention for "Private, don't use".

"Somebody" could easily be the class designer. This is a method that, as
written, will break the instance if it is run twice. That's not good.

 
>> Some people might argue that if someone runs a private method like
>> _accumulate_properties, they deserve whatever bad things happen to
>> them.  But I say, well, sure, but why *design* your method to break
>> things when called twice?
> 
> Exactly the same could be said for calling the __init__ method twice.

__init__ isn't a "private" method, it is a public method. A very special
public method, designed to initialise the instance, that is to set the
instance to a brand new pristine state.

Calling instance.__init__ *should* initialise the instance and throw away
whatever data was already there. (Why you would want to do this is another
question.) What else would you expect it to do? If you called __init__ and
it *didn't* initialise the instance, that would surely be a bug.

But _accumulate_properties as written is sort of half-man-half-duck sort
of thing: it would be useful for accumulating properties to the instance,
except that when you do so it throws away whatever properties you have
previously accumulated. Or rather, it doesn't even do that -- it simply
throws away the list of properties, but leaves the actual properties
behind. That can't possibly be good design! An inconsistent internal
state like that is a bug waiting to happen.


>> Who knows, maybe you'll decide you want to call it twice yourself.
> 
> Right. In which case, it's good that it's already in a separate,
> clearly-named, single-purpose method.

It isn't either clearly-named or single-purpose.

The name is misleading, for two reasons. First, "properties" has a
technical meaning in Python, and this method doesn't have anything to do
with that meaning. Secondly, even if it did, it doesn't accumulate them,
it resets the list to a new state, throwing away whatever was there before. 

Nor is it single-purpose: while it *sets* a list of attribute names to a
new list of values, it *adds* those new attributes to the instance __dict__.

There are some methods that, in principle, should be run once and once
only. There's nothing inherently about accumulating attributes/properties
to an instance can only be done once. But the way the method is written,
it will break things if you try to accumulate attributes twice -- not
because of any inherent badness in doing so, but because the method breaks
the consistency between self.properties (a list) and the actual attributes
accumulated.

> Make the code easy to understand, not idiot-proof.

I'm not arguing that the entire method should be moved into __init__.
*Initialising* self.properties list to [] is not logically part of
*accumulating* properties, and should be moved out of that method. That
tiny change will fix all of the problems I describe except for the
misleading name.



-- 
Steven D'Aprano