From: John M. <jo...@eb...> - 2012-11-23 17:39:32
|
Hi All, This has been on my todo list since the last developer hangout. In the most recent release (1.14.15) the cloning of stereo elements was added. For the cloning a deep copy is returned as expected but for the shallow I mimicked the behaviour of the bonds. The purpose of this mail is to ensure the behaviour is known and also see if it is what is expected. Here is the copy constructor behaviour https://gist.github.com/4136504 1 2 3 4 5 6 7 8 9 10 11 12 13 14 IAtomContainer a = new AtomContainer(); a.addAtom(new Atom("C")); a.addAtom(new Atom("C")); a.addBond(new Bond(a.getAtom(0), a.getAtom(1), SINGLE)); // copy constructor IAtomContainer b = new AtomContainer(a); b.removeAtom(0); // atom at index 0 is removed from b, a is not affected b.getBond(0).setOrder(DOUBLE); // bond order is change in both a and b b.getBond(0).addAtom(new Atom("H")); // as the bond is the same instance this is now ITetrahedralChiralAtomParityEtc element = ...; // access stereo elements element.setStereo(CLOCKWISE); // a,b both have stereo changed Thinking about the front end uses it might make more sense to shallow copy the bonds/stereo elements as well as the container or even make the copy constructor perform a deep copy. Most cases I've seen where a method has needed a copy they tend create there own method, specific to their needs or use a full clone when it isn't needed. So, does the behaviour shown above make sense? Perhaps the copy constructor should be a deep copy constructor… maybe there should be a 'deepCopy()' method (everyone knows clone is evil and should not be used). There is of course no right answer but having seen a lot defensive cloning and copy pasted reimplementations it might be that the currently implementation isn't that useful. Please let me know your thoughts. Many thanks, John |