On Nov 27, 2007, at 7:53 AM, Egon Willighagen wrote:
> Indeed, there is a JUnit test which looks like:
>
> IChemObject chemObject1 = builder.newChemObject();
> Hashtable props1 = new Hashtable();
> IAtom atom = builder.newAtom("C");
> props1.put("atom", atom);
> chemObject1.setProperties(props1);
> IChemObject chemObject2 = (IChemObject)chemObject1.clone();
>
> // test cloning of properties field
> Hashtable props2 = new Hashtable();
> chemObject2.setProperties(props2);
> Assert.assertEquals(props1, chemObject1.getProperties());
> Assert.assertEquals(1, chemObject2.getProperties().size());
^^
Why does the above assertion succeed? chemobject2 is a clone of
chemobject1, but you then set the properties of chemobject2 to an
empty Hashtable. So the size of the hash table that is returned
should be 0
> Assert.assertEquals(1, chemObject1.getProperties().size());
> // ok, copied hashtable item, but this item should be cloned
> Assert.assertNotSame(atom, chemObject2.getProperties().get
> ("atom"));
Similarly how does the last assertion above work? chemobject2 is
supposed to have an empty hash table as it's properties
>
> The last line requires the "atom" property of the cloned atom not to
> be the same instance as the original, that is a clone.
>
> So, there needs to be a API change issued to solve this bug. I have
> run all JUnit tests for the library, and the not cloning
> of properties does not give any failing unit tests other than the one
> just discussed.
>
> However, the user community should indicate what the expected behavior
> of clone() with respect to IChemObject type properties should be.
>
> I cannot remember why I (commit rev 3087) added the cloning of
> IChemObjects, and would actually propose to not clone these
> properties.
> Alternatively, IChemObject type properties can be dropped... one could
> question of the usefulness of cloning in "myParent" property use
> cases.
> So, there at least three options, and we need to decide which one the
> CDK should adopt:
>
> 1. clone IChemObject type properties, using some cycle detection
> and max depth
> 2. not clone IChemObject type properties
> 3. drop any IChemObject type properties
3) is attractive since then only if a specific subclass of
IChemObject requires properties it would implement this facility.
However, this option seems like it would lead to breakage especially
if certain classes are going to work with an IChemObject (rather than
the specific subclass) and expect that properties will be available.
Related to this approach, consider something like Molecule - one
could create a similar scenario as we currently have for ChemObject.
That is a property of Molecule could refer to the Molecule object,
leading to a similar stack overflow problem. So I'm not sure if 3)
would be a valid solution
2) is probably the easiest way out, but it's something that the
library user then has to keep in mind - since it is intuitive that if
an object is cloned, *all* components of the object are also cloned.
But ChemObject.clone() already notes in the Javadocs:
The ChemObjectListeners are not cloned, and
neither is the content of the pointer vectors.
So we'd just update this message.
But it does lead to a problem on the user side, since then the user
must properly clone the properties. More significantly, if the
property refers to parts of the original ChemObject, then the cloned
property must refer to the correct parts of the cloned ChemObject.
It seems that 1) would be a little more involved in terms of coding,
but also, by specifying a max depth, one might fail to properly clone
an object in some cases (especially if max depth is set to a default).
However all this is only relevant when the property is equal to the
actual parent object - for components of a parent object I don't see
that this is a problem. This observation could be used to simplify
option 1) leading to a clone method that looks like
clone() throws .... {
ChemObject clone = (ChemObject)super.clone();
// get properties
for key, property in properties
if property.equals(this):
cloneProperties.put(key.clone(), clone)
...
return clone
}
-------------------------------------------------------------------
Rajarshi Guha <[EMAIL PROTECTED]>
GPG Fingerprint: 0CCA 8EE2 2EEB 25E2 AB04 06F7 1BB9 E634 9B87 56EE
-------------------------------------------------------------------
C Code.
C Code Run.
Run, Code, RUN!
PLEASE!!!!
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Cdk-user mailing list
Cdk-user@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/cdk-user
