parent-child object design question

[manstey]

Hi,

I am having trouble designing my classes.

I have two classes. The first one wraps around an old-style class
called oref
Class CacheClass(obje ct):
def __init__(self, obj):
self.__data = obj
def __getattr__(sel f, attr):
return getattr(self.__ data, attr)

The second class is much the same, also wrapping, but has some
attributes.

class CacheProperty(o bject):
def __init__(self, obj, dicProperties={ }):
self.__data = obj
lisProperties=[]
for key, val in dicProperties.i teritems():
setattr(self, key, val)
lisProperties.a ppend(key)
self.Properties = lisProperties
def __getattr__(sel f, attr):
return getattr(self.__ data, attr)

Here is my code:

>>MyClass = CacheClass(oref )
MyProperty = CacheProperty(o ref2,{'Name':'S urname', 'Value':'Peter' })
setattr(MyCla ss,MyProperty.N ame,MyProperty)

Now, the problem is that I want a method MyClass.MyPrope rty.Save() to
save the value of MyClass.MyPrope rty to the database backend on disk,
but the code for this is part of the wrapped oref code, and thus is
invoked only by MyClass.set(MyP roperty.Name,My Property.Value) .

How can I access this method inside the MyProperty class?

I hope this is clear!

regard,s
matthew

[Ben Finney]

"manstey" <ma*****@csu.ed u.auwrites:

I have two classes. The first one wraps around an old-style class
called oref

Class CacheClass(obje ct):
def __init__(self, obj):
self.__data = obj
def __getattr__(sel f, attr):
return getattr(self.__ data, attr)

I presume the 'obj' argument to this class's __init__ method contains
the 'oref' instance.

It's a little confusing to call the argument to __getattr__ "attr",
since it's actually the *name* of the attribute to be accessed, not
the attribute itself.

The second class is much the same, also wrapping, but has some
attributes.

class CacheProperty(o bject):
def __init__(self, obj, dicProperties={ }):

Setting a container class as a default argument is prone to error; the
default argument gets created once, when the 'def' statement is
executed. Better to default to None, and trigger on that inside the
function.

self.__data = obj
lisProperties=[]
for key, val in dicProperties.i teritems():
setattr(self, key, val)
lisProperties.a ppend(key)
self.Properties = lisProperties

The name of this class doesn't really tell me what an instance of the
class *is*. Is it a "cache property", as the name seems to indicate?
If so, why does a "cache property" instance itself contain a list of
properties?

def __getattr__(sel f, attr):
return getattr(self.__ data, attr)

Here is my code:

>MyClass = CacheClass(oref )

This is a confusing example; MyClass implies that the object is a
class, but this is now an instance of CacheClass, not a class.

Also, it's conventional in Python to name classes in TitleCase, but to
name instances (and functions) starting with lowercase.

>MyProperty = CacheProperty(o ref2,{'Name':'S urname', 'Value':'Peter' })
setattr(MyClas s,MyProperty.Na me,MyProperty)

This might be a good approach if you didn't know you were going to
associate the object MyClass with the object MyProperty at the
creation of MyProperty. However:

Now, the problem is that I want a method MyClass.MyPrope rty.Save()
to save the value of MyClass.MyPrope rty to the database backend on
disk, but the code for this is part of the wrapped oref code, and
thus is invoked only by
MyClass.set(MyP roperty.Name,My Property.Value) .

This implies that there's a definite "each CacheProperty instance is
associated with exactly one CacheClass instance" invariant.

If that's true, the best thing to do is to pass MyClass to the
constructor for the CacheProperty class, and have each instance set
the relationship in the __init__ method.

I don't know what a descriptive term for the relationship between the
CacheClass instance and the CacheProperty instance is, so I'm going to
use "parent"; you should choose something more descriptive.

class CacheProperty(o bject):
def __init__(self, obj, parent, properties=None ):
self.__data = obj
self._bind_to_p arent(parent)
if properties is None:
properties = {}
self._accumulat e_properties(pr operties)

def _bind_to_parent (self, parent):
setattr(parent, self.Name, self)

def _accumulate_pro perties(self, properties):
self.properties = []
for key, val in properties.iter items():
setattr(self, key, val)
self.properties .append(key)

def __getattr__(sel f, name):
return getattr(self.__ data, name)

--
\ "Marriage is a wonderful institution, but who would want to |
`\ live in an institution." -- Henry L. Mencken |
_o__) |
Ben Finney

[manstey]

Hi Ben,

Could I also do something like the following? What does it mean to
store the parent class as a private variable in the child class?

class CacheProperty(o bject):
def __init__(self, obj, parent, properties=None ):
self.__data = obj
self._parent = parent
if properties is None:
properties = {}
self._accumulat e_properties(pr operties)
def _accumulate_pro perties(self, properties):
self.properties = []
for key, val in properties.iter items():
setattr(self, key, val)
self.properties .append(key)
def __getattr__(sel f, name):
return getattr(self.__ data, name)

def set(self, property):
return self._parent.se t(property)

the set function allows us to call the parent set function within the
child, which is what we need to do.

[Ben Finney]

"manstey" <ma*****@csu.ed u.auwrites:

Could I also do something like the following?

I can't immediately see a problem with the code you posted. Does it do
what you want it to do?

What does it mean to store the parent class as a private variable in
the child class?

I don't understand this question. It's up to you what it means; I
leave it to the people who know the purpose of the program to decide
whether "parent" is even an appropriate term.

--
\ "I took a course in speed waiting. Now I can wait an hour in |
`\ only ten minutes." -- Steven Wright |
_o__) |
Ben Finney

[Steven D'Aprano]

On Tue, 30 Jan 2007 21:15:53 -0800, manstey wrote:

Hi Ben,

Could I also do something like the following? What does it mean to
store the parent class as a private variable in the child class?

What it means is that references to "self.__dat a" (note the TWO leading
underscores) in your code below are mangled by Python to be
"self._CachePro perty__data__" instead.

This gives you a weak form of "private" variable, good for annoying people
and yourself, possibly okay to help prevent many accidental name clashes,
but not much else.

On the other hand, references to "self._pare nt" (note only ONE leading
underscore) is a convention. It is like putting a comment "Private, don't
touch" in your code. It isn't enforced by Python. That's usually a good
thing.

class CacheProperty(o bject):
def __init__(self, obj, parent, properties=None ):
self.__data = obj
self._parent = parent
if properties is None:
properties = {}
self._accumulat e_properties(pr operties)
def _accumulate_pro perties(self, properties):
self.properties = []

Probably better to put that in the __init__ method, otherwise if somebody
runs instance._accum ulate_propertie s(...) again, it will have the
side-effect of throwing away whatever was already in instance.proper ties.

Some people might argue that if someone runs a private method like
_accumulate_pro perties, they deserve whatever bad things happen to them.
But I say, well, sure, but why *design* your method to break things when
called twice? Who knows, maybe you'll decide you want to call it twice
yourself.

for key, val in properties.iter items():
setattr(self, key, val)
self.properties .append(key)

If I am reading this correctly, self.properties is almost the same as
self.__dict__.k eys() only missing some values.

def __getattr__(sel f, name):
return getattr(self.__ data, name)

Okay, let's see how this works.

child = CacheProperty([1,2,3], PARENT, {"food": "eggs", "colour": "green"})

# child.__data is the list [1,2,3]
# child._parent is PARENT (whatever that is)
# child.propertie s is the list ['food', 'colour']

Now, if I say:

child.__len__()
=returns 3

child.index(2)
=returns 1

That's just straight-forward delegation to the "obj" argument stored in
data. But when I say:

child.set('foo' )

expecting to have the following method called:

def set(self, property):
return self._parent.se t(property)

oops! It gets delegated to obj instead of the parent.

the set function allows us to call the parent set function within the
child, which is what we need to do.

Why not do this?

instance._paren t.set('foo')

No mess, no fuss, bug-free, and self-documenting.
--
Steven D'Aprano

[Ben Finney]

"Steven D'Aprano" <st***@REMOVEME .cybersource.co m.auwrites:

def _accumulate_pro perties(self, properties):
self.properties = []

Probably better to put that in the __init__ method, otherwise if
somebody runs instance._accum ulate_propertie s(...) again, it will
have the side-effect of throwing away whatever was already in
instance.proper ties.

That's the point of naming it with a leading underscore. You've
already explained that this is convention for "Private, don't use".

Some people might argue that if someone runs a private method like
_accumulate_pro perties, 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.

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.

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

--
\ "One time a cop pulled me over for running a stop sign. He |
`\ said, 'Didn't you see the stop sign?' I said, 'Yeah, but I |
_o__) don't believe everything I read.'" -- Steven Wright |
Ben Finney

[manstey]

Thanks for your input. Here is my next version, which works very well,
but for one problem I explain below:

class CacheProperty(o bject):
def __init__(self, insCacheClass, name):
self.Name = name
self._bind_to_p arent(insCacheC lass)
self.__parent = insCacheClass
self.__pyValue = self.__parent.g et(name)

def _bind_to_parent (self, parent):
setattr(parent, self.Name, self)

def get(self):
return self.__pyValue

def set(self, val):
self.__parent.s et(self.Name, val)
self.__pyValue = val # Set in wrapper's copy

Value = property(get, set)

def __repr__(self):
return str(self.Value)
def __str__(self):
return str(self.Value)
class CacheClass(obje ct):
def __init__(self, obj):
self.__data = obj

def getOref(self):
return self.__data
def __repr__(self):
return 'self.__data'
def __str__(self):
return str(self.__data )
def __getattr__(sel f, attr):
return getattr(self.__ data, attr)

Our code works fine as follows (oref is in-memory version of Cache oo-
dbase class, and is an old-style class that comes with set and get and
run_obj_method methods):

>>insCacheCla ss = CacheClass(oref )
insCachePrope rty = CacheProperty(i nsOref,'Chapter ')
print insOref.Chapter .Value

5

>>print insOref.Chapter .Name

'Chapter'

>>insOref.Chapt er.Value=67
print insOref.Chapter

67

However, the problem is now that I can also write:

>>insOref.Chapt er=67

but we want to disallow this, as insOref.Chapter must remain =
insProperty

We add various other instances of CacheProperty to the insOref as
well, btw.

So any idea how to prohibit this, and can the class code above be
improved? Does it matter that each instance of CacheProperty contains
self.__parent? Does it actually "contain" the parent (I realise this
is not 'parent' in usual python lingo - what would be a better term?),
or is self.__parent simply point to it somehow? I don't understand
this part of python at all!

Thanks, you are being a great help in our development.

[Ben Finney]

"manstey" <ma*****@csu.ed u.auwrites:

However, the problem is now that I can also write:

>insOref.Chapte r=67

but we want to disallow this, as insOref.Chapter must remain =
insProperty

Then don't do that.

Python allows any name to be reassigned to any value, with the
attitude of "we're all consenting adults here". It's better to
document[0] the correct behaviour of
your code, rather than trying to prevent stupid mistakes.
[0]: and unit tests, that explicitly check the behaviour of the code,
and get run all the time during development of the code, are the best
way of documenting behaviour unambiguously.

--
\ "Buy not what you want, but what you need; what you do not need |
`\ is expensive at a penny." -- Cato, 234-149 BC, Relique |
_o__) |
Ben Finney

[Steven D'Aprano]

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

"Steven D'Aprano" <st***@REMOVEME .cybersource.co m.auwrites:

def _accumulate_pro perties(self, properties):
self.properties = []

Probably better to put that in the __init__ method, otherwise if
somebody runs instance._accum ulate_propertie s(...) again, it will
have the side-effect of throwing away whatever was already in
instance.prope rties.

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_pr operties, 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_pro perties 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

[Steven D'Aprano]

On Wed, 31 Jan 2007 15:09:29 -0800, manstey wrote:

Thanks for your input. Here is my next version, which works very well,
but for one problem I explain below:

class CacheProperty(o bject):
def __init__(self, insCacheClass, name):
self.Name = name
self._bind_to_p arent(insCacheC lass)
self.__parent = insCacheClass
self.__pyValue = self.__parent.g et(name)

def _bind_to_parent (self, parent):
setattr(parent, self.Name, self)

def get(self):
return self.__pyValue

def set(self, val):
self.__parent.s et(self.Name, val)
self.__pyValue = val # Set in wrapper's copy

Value = property(get, set)

def __repr__(self):
return str(self.Value)
def __str__(self):
return str(self.Value)
class CacheClass(obje ct):
def __init__(self, obj):
self.__data = obj

def getOref(self):
return self.__data

That's pointless. Just get rid of the double-underscores and let the
caller refer to self.data directly.

def __repr__(self):
return 'self.__data'

What's the purpose of this? Why do completely different instances of
CacheClass have exactly the same repr?

def __str__(self):
return str(self.__data )

Pointless, because __getattr__ will have the same effect: if __str__
doesn't exist, __getattr__ will call getattr(self.__ data, "__str__") which
is equivalent to str(self.__data ).

def __getattr__(sel f, attr):
return getattr(self.__ data, attr)

Straight-forward delegation to self.__data. As near as I can see,
CacheClass(oref ) essentially does nothing different to just oref. So why
wrap oref in another class? What's the purpose of this? Why not just refer
to oref in the first place?

What makes this a "cache" class? It doesn't seem to act anything like a
cache to me. It seems to be just a wrapper around oref that doesn't really
do anything.

Our code works fine as follows (oref is in-memory version of Cache oo-
dbase class, and is an old-style class that comes with set and get and
run_obj_method methods):

>>>insCacheClas s = CacheClass(oref )
insCacheProp erty = CacheProperty(i nsOref,'Chapter ')

What is insOref and where does it come from? Is it different from oref?

Your classes are confusing, but I feel that all of the above is just a
big, complicated way of saying something like this:

insOref = something_or_ot her
insOref.some_na me = insOref

and then jumping through all sorts of hoops writing this:

insOref.some_na me.attribute

instead of just insOref.attribu te
That's what it seems like to me. Have I missed something?

>>>print insOref.Chapter .Value

5

>>>print insOref.Chapter .Name

'Chapter'

>>>insOref.Chap ter.Value=67
print insOref.Chapter

67

However, the problem is now that I can also write:

>>>insOref.Chap ter=67

but we want to disallow this, as insOref.Chapter must remain =
insProperty

Why?

We add various other instances of CacheProperty to the insOref as
well, btw.

This whole design seems incredibly byzantine to me. It would help if you
take a step back from asking technical questions "how do I do this?" and
instead tell us what you're trying to accomplish.

Why do you wrap the instances in the first place? What benefit do you get?

Here is something that you should do. Write your documentation for the
classes CacheProperty and CacheClass. The documentation should explain
what they are for, why you would use them, and how you use them. If you
can't explain what they are for and why you would use them, chances are
very very good that they don't do anything useful and you shouldn't use
them at all.


--
Steven D'Aprano