From: Egon W. <eg...@us...> - 2004-08-10 14:29:35
|
Hi all, recently I posted some bits on the PMD tests, and some remaining problems with the core classes... I might have found another one... What about cloning? Therefore: =========================================== Request for Comments Name: Clones and Copies Date: 2004-08-07 Proposer: Egon Willighagen REASON AtomContainer has a shallowCopy() and a clone()... the first clones everything by making clones of all fields, while the second only creates a new AtomContainer, but with the exact same instances for its content... Atom's clone() method, however, was more like a shallowCopy... I've fixed this, but since this is so important I really want to make this a formal agreement, and therefore this proposal. PROPOSAL Each core class has a clone() and a copy() method. The first makes a deep clone, which means that clone() makes a clone of itself and all of its content. The copy() method, however, makes a clone of itself and maintains the instances of all object fields. =========================================== -- eg...@us... GPG: 1024D/D6336BA6 |
From: Kai H. <Kai.Hartmann@Uni-Koeln.De> - 2004-08-12 11:44:23
|
Hi all, I agree with your proposal. But: The copy() method should be named shallowCopy() - as it is now. This way it is obvious for the once-in-a-while user what the methods do. Kai Egon Willighagen wrote: > Hi all, > > recently I posted some bits on the PMD tests, and some remaining problems with > the core classes... I might have found another one... > > What about cloning? Therefore: > > =========================================== > Request for Comments > > Name: Clones and Copies > Date: 2004-08-07 > Proposer: Egon Willighagen > > REASON > > AtomContainer has a shallowCopy() and a clone()... the first clones everything > by making clones of all fields, while the second only creates a new > AtomContainer, but with the exact same instances for its content... > > Atom's clone() method, however, was more like a shallowCopy... I've fixed > this, but since this is so important I really want to make this a formal > agreement, and therefore this proposal. > > PROPOSAL > > Each core class has a clone() and a copy() method. The first makes a deep > clone, which means that clone() makes a clone of itself and all of its > content. The copy() method, however, makes a clone of itself and maintains > the instances of all object fields. > > =========================================== > |
From: Christoph S. <c.s...@un...> - 2004-08-12 13:58:32
|
Kai Hartmann wrote: > I agree with your proposal. But: The copy() method should be named > shallowCopy() - as it is now. This way it is obvious for the > once-in-a-while user what the methods do. I agree with this (Of course, because I originally named the method like this). For a great writeup of the issue, I recommend Bruce Eckel's magnificent book "Thinking in Java" (http://www.planetpdf.com/codecuts/pdfs/eckel/TIJ3.zip). On page 1053, he explains cloning and copying in great detail. Cheers, Chris > Egon Willighagen wrote: > >> Hi all, >> >> recently I posted some bits on the PMD tests, and some remaining >> problems with the core classes... I might have found another one... >> What about cloning? Therefore: >> >> =========================================== >> Request for Comments >> >> Name: Clones and Copies >> Date: 2004-08-07 >> Proposer: Egon Willighagen >> >> REASON >> >> AtomContainer has a shallowCopy() and a clone()... the first clones >> everything by making clones of all fields, while the second only >> creates a new AtomContainer, but with the exact same instances for its >> content... >> Atom's clone() method, however, was more like a shallowCopy... I've >> fixed this, but since this is so important I really want to make this >> a formal agreement, and therefore this proposal. >> >> PROPOSAL >> >> Each core class has a clone() and a copy() method. The first makes a >> deep clone, which means that clone() makes a clone of itself and all >> of its content. The copy() method, however, makes a clone of itself >> and maintains the instances of all object fields. >> >> =========================================== >> > > > ------------------------------------------------------- > SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media > 100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33 > Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift. > http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285 > _______________________________________________ > Cdk-devel mailing list > Cdk...@li... > https://lists.sourceforge.net/lists/listinfo/cdk-devel > > > |
From: E.L. W. <E.W...@sc...> - 2004-08-12 14:14:59
|
=2D----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 12 August 2004 15:58, Christoph Steinbeck wrote: > Kai Hartmann wrote: > > I agree with your proposal. But: The copy() method should be named > > shallowCopy() - as it is now. This way it is obvious for the > > once-in-a-while user what the methods do. > > I agree with this (Of course, because I originally named the method like > this). For a great writeup of the issue, I recommend Bruce Eckel's > magnificent book "Thinking in Java" > (http://www.planetpdf.com/codecuts/pdfs/eckel/TIJ3.zip). > On page 1053, he explains cloning and copying in great detail. Ok, I go ahead and make the changes. With luck I can release a new CDK next= =20 weekend... Egon =2D --=20 E.W...@sc... PhD on Molecular Representation in Chemometrics Radboud University, Nijmegen http://www.cac.sci.ru.nl/people/egonw/ GPG: 1024D/D6336BA6 "Again a chemist did something useful with a computer" =2D----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (SunOS) iD8DBQFBG3tYd9R8I9Yza6YRAnL+AKC0T1gakWSDyCtCSpeMrD08dd2xNACfTFOA tDAG77gUoAOR0jBaX9lTYDo=3D =3D1XTj =2D----END PGP SIGNATURE----- |
From: Kai H. <Kai.Hartmann@Uni-Koeln.De> - 2004-08-12 14:45:35
|
Hi all, now that a few changes are made to the API and the behaviour of methods=20 (see clone and copy), I would like to suggest a check of the cdk method=20 names and class names (core only) for consistency. 1. One example: AtomContainer.getAtomAt(int) AtomContainer.getBondAt(int) <-> SetOfAtomContainers.getAtom(int) SetOfMolecule.getMolecule(int) I always forget the "at" in the first two method names :-) 2. Another one: RingSet <-> SetOfMolecules, SetOfAtomContainers But maybe that is intended (RingSet extends Vector, SetOfBla not). 3. I would also prefer every class to have a "size" convenience method=20 in addition to the getBlaCount, and maybe also a "get" (e.g.=20 AtomContainer.get(int) in addition to getAtom). I know that changes to the core of cdk would break functionality of many=20 existing projects. And I don't know what you think about deprecated metho= ds. Comments are very much appreciated, Kai --=20 Kai Hartmann CUBIC - Cologne University BioInformatics Center Institute for Biochemistry Z=FClpicher Strasse 47 50674 Koeln Phone: +49-221-470-7719 Fax: +49-221-470-7786 |
From: Uli F. <u.f...@ch...> - 2004-08-12 15:39:26
|
> 1. One example: > AtomContainer.getAtomAt(int) > AtomContainer.getBondAt(int) > <-> > SetOfAtomContainers.getAtom(int) > SetOfMolecule.getMolecule(int) > > I always forget the "at" in the first two method names :-) I second that. A meaningful unification of function names as in the above example makes sense. > 2. Another one: > RingSet <-> SetOfMolecules, SetOfAtomContainers > > But maybe that is intended (RingSet extends Vector, SetOfBla not). Even though this breaks a lot of functionality it might be worth it... > 3. I would also prefer every class to have a "size" convenience method > in addition to the getBlaCount, and maybe also a "get" (e.g. > AtomContainer.get(int) in addition to getAtom). I do not see the point of having a size method for every class. In same cases it may be obvious what size is; in other cases it is not. > I know that changes to the core of cdk would break functionality of many > existing projects. And I don't know what you think about deprecated > methods. Redesign always breaks code functionality. But as this project grew for some time now, maybe it is a good idea to think about the basic design (in this case: function/class names). Uli |
From: E.L. W. <eg...@us...> - 2004-08-12 15:52:22
|
=2D----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 12 August 2004 17:39, Uli Fechner wrote: > Even though this breaks a lot of functionality it might be worth it... I can asure everyone that API has been broken many times in the past few=20 years... less often in the core classes. Just browse through the ChangeLog and look for the sections called 'API Changes'. If just a method name changes, that's not really a big deal. I've done plen= ty of search/replace on the whole CDK tree... It's far more difficult if the=20 functionality of the method changes... If we all agree on some method name changes, than I'm perfectly fine with=20 that... Remember that not too far ago, the core classes underwent such a method name change: getPoint2D() -> getPoint2d(), to match the name of the vecmath= =20 class Point2d... There are a reasonably high number of JUnit tests, to allow for such a=20 relatively save modification... Egon =2D --=20 eg...@us... =2D --------------------------------------- CDK: http://cdk.sf.net/ JChemPaint: http://jchempaint.sf.net/ Jmol: http://www.jmol.org/ =2D----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (SunOS) iD8DBQFBG5Iud9R8I9Yza6YRAvHEAKDGKWaeDT6nNg0VLSF8DGOJHW0hvQCfYWtB w4sRqNkbHzNHq47oQ0wiM1k=3D =3DjhNO =2D----END PGP SIGNATURE----- |
From: E.L. W. <eg...@us...> - 2004-08-13 08:14:50
|
=2D----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 12 August 2004 16:42, Kai Hartmann wrote: > 1. One example: > AtomContainer.getAtomAt(int) > AtomContainer.getBondAt(int) > <-> > SetOfAtomContainers.getAtom(int) > SetOfMolecule.getMolecule(int) > > I always forget the "at" in the first two method names :-) Ok, we indeed need to synch this... What would be preferred? With At or=20 without? > 2. Another one: > RingSet <-> SetOfMolecules, SetOfAtomContainers > > But maybe that is intended (RingSet extends Vector, SetOfBla not). Ok, if I understand correctly, your point here is that RingSet extends from= =20 Vector to hold Ring's which extend AtomContainer, so RingSet should really= =20 extend SetOfAtomContainers, correct? If so, I second that. This patch would be slightly (understatement) larger= =20 than the first... but achievable... Egon =2D --=20 eg...@us... =2D --------------------------------------- CDK: http://cdk.sf.net/ JChemPaint: http://jchempaint.sf.net/ Jmol: http://www.jmol.org/ =2D----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (SunOS) iD8DBQFBHHh1d9R8I9Yza6YRAnSKAJ99kAb9YstjN9YT3AKkjRTNKAjGkwCglgSf llduJBXkn5tRF6kdc+4FvHw=3D =3DCPKR =2D----END PGP SIGNATURE----- |
From: Kai H. <Kai.Hartmann@Uni-Koeln.De> - 2004-08-13 08:27:49
|
E.L. Willighagen wrote: > On Thursday 12 August 2004 16:42, Kai Hartmann wrote: > >>>1. One example: >>>AtomContainer.getAtomAt(int) >>>AtomContainer.getBondAt(int) >>><-> >>>SetOfAtomContainers.getAtom(int) >>>SetOfMolecule.getMolecule(int) >>> >>>I always forget the "at" in the first two method names :-) > > > Ok, we indeed need to synch this... What would be preferred? With At or > without? without. > >>>2. Another one: >>>RingSet <-> SetOfMolecules, SetOfAtomContainers >>> >>>But maybe that is intended (RingSet extends Vector, SetOfBla not). > > > Ok, if I understand correctly, your point here is that RingSet extends from > Vector to hold Ring's which extend AtomContainer, so RingSet should really > extend SetOfAtomContainers, correct? > > If so, I second that. This patch would be slightly (understatement) larger > than the first... but achievable... :) Yes, RingSet extending SetOfAtomContainers would be nice. I wasn't sure if there was a requirement for it to extend Vector. BTW, I like the shorter version "RingSet" better than "SetOfRings", but this is just a matter of taste. Renaming all SetOfXXXs classes to XXXSet would be the larger change... Kai |
From: Uli F. <u.f...@ch...> - 2004-08-13 12:15:50
|
> BTW, I like the shorter version "RingSet" better than "SetOfRings", but > this is just a matter of taste. Renaming all SetOfXXXs classes to XXXSet > would be the larger change... > > Kai > I would prefer the shorter names (XXXSet), too. Having a consistent name scheme for classes and functions may be really important. Especially if you are a beginner it makes life a lot easier... Uli |
From: Christoph S. <c.s...@un...> - 2004-08-14 12:27:03
|
Kai Hartmann wrote: > Yes, RingSet extending SetOfAtomContainers would be nice. I wasn't sure= =20 > if there was a requirement for it to extend Vector. There is a historical reason, with RingSet being older than=20 SetOfAtomContainers. Cheers, Chris --=20 Dr. rer. nat. habil. Christoph Steinbeck (c.s...@un...) Groupleader Junior Research Group for Applied Bioinformatics Cologne University BioInformatics Center (http://www.cubic.uni-koeln.de) Z=FClpicher Str. 47, 50674 Cologne Tel: +49(0)221-470-7426 Fax: +49 (0) 221-470-7786 What is man but that lofty spirit - that sense of enterprise. ... Kirk, "I, Mudd," stardate 4513.3.. |
From: E.L. W. <eg...@us...> - 2004-08-12 17:02:58
|
=2D----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 12 August 2004 16:14, E.L. Willighagen wrote: > On Thursday 12 August 2004 15:58, Christoph Steinbeck wrote: > > I agree with this (Of course, because I originally named the method like > > this). For a great writeup of the issue, I recommend Bruce Eckel's > > magnificent book "Thinking in Java" > > (http://www.planetpdf.com/codecuts/pdfs/eckel/TIJ3.zip). > > On page 1053, he explains cloning and copying in great detail. > > Ok, I go ahead and make the changes. With luck I can release a new CDK ne= xt > weekend... That PDF is very good reading material! I second Chris' recommendation... Anyway, I've been working on implementing the proposal, and turns out to be rather tedious... so I will have to be very lucky to make a release this=20 weekend :) I also already found a bug in the core classes, so my efforts will not go=20 unrewarded, no matter wether I'll complete the clone()/shallowCopy() code o= r=20 not :) Egon =2D --=20 eg...@us... =2D --------------------------------------- CDK: http://cdk.sf.net/ JChemPaint: http://jchempaint.sf.net/ Jmol: http://www.jmol.org/ =2D----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (SunOS) iD8DBQFBG6K+d9R8I9Yza6YRAkZ9AJ9acD/1Sf/kxzFjizk6mpyA1Mxf4QCeOwDI 43iEL+HF9g9dHNdaGeAuwQc=3D =3DNUi4 =2D----END PGP SIGNATURE----- |
From: E.L. W. <eg...@us...> - 2004-08-23 11:50:54
|
=2D----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 12 August 2004 16:14, E.L. Willighagen wrote: > On Thursday 12 August 2004 15:58, Christoph Steinbeck wrote: > > Kai Hartmann wrote: > > > I agree with your proposal. But: The copy() method should be named > > > shallowCopy() - as it is now. This way it is obvious for the > > > once-in-a-while user what the methods do. > > > > I agree with this (Of course, because I originally named the method like > > this). For a great writeup of the issue, I recommend Bruce Eckel's > > magnificent book "Thinking in Java" > > (http://www.planetpdf.com/codecuts/pdfs/eckel/TIJ3.zip). > > On page 1053, he explains cloning and copying in great detail. > > Ok, I go ahead and make the changes. With luck I can release a new CDK ne= xt > weekend... Ok, two weeks later... I think I've got all core classes correctly implemen= ted=20 now... many core classes have extensive JUnit tests for the clone() method,= =20 but not all... it could do with more tests, so if you have some spare time,= =20 please join #cdk, and ask which clone test you could implement... I plan to do a new CDK release next weekend, so please check out and try the CDK version is CVS now. It should be as stable as the last release... likel= y=20 even more stable, because CVS contains a number of bug fixes compared with= =20 the 20040626 release... But mind the API changes! Egon =2D --=20 eg...@us... =2D --------------------------------------- CDK: http://cdk.sf.net/ JChemPaint: http://jchempaint.sf.net/ Jmol: http://www.jmol.org/ =2D----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (SunOS) iD8DBQFBKdoVd9R8I9Yza6YRAnMfAJ9ITkXzSr1s5fOo/dvYESJtzO+6YgCghEYn R/QmKkzo1Ff5fIKApZn99/I=3D =3DuFIY =2D----END PGP SIGNATURE----- |
From: Stefan K. <ste...@un...> - 2004-08-12 15:25:16
|
Hi, I think a certan degree of unification is ok, even if it breaks some=20 functions. The get...(At) methods should be uniform. I don't like the ide= a of=20 having a lot of methods doing the same (like get and getAtom). I think we= =20 should also keep names descriptive. So getSize is not good, since what is= the=20 size of a molecule? Stefan Am Thursday 12 August 2004 16:42 schrieb Kai Hartmann: > Hi all, > > now that a few changes are made to the API and the behaviour of methods > (see clone and copy), I would like to suggest a check of the cdk method > names and class names (core only) for consistency. > > 1. One example: > AtomContainer.getAtomAt(int) > AtomContainer.getBondAt(int) > <-> > SetOfAtomContainers.getAtom(int) > SetOfMolecule.getMolecule(int) > > I always forget the "at" in the first two method names :-) > > > 2. Another one: > RingSet <-> SetOfMolecules, SetOfAtomContainers > > But maybe that is intended (RingSet extends Vector, SetOfBla not). > > > 3. I would also prefer every class to have a "size" convenience method > in addition to the getBlaCount, and maybe also a "get" (e.g. > AtomContainer.get(int) in addition to getAtom). > > I know that changes to the core of cdk would break functionality of man= y > existing projects. And I don't know what you think about deprecated > methods. > > Comments are very much appreciated, > > Kai --=20 Stefan Kuhn M. A. Cologne University BioInformatics Center (http://www.cubic.uni-koeln.de) Z=FClpicher Str. 47, 50674 Cologne Tel: +49(0)221-470-7428 Fax: +49 (0) 221-470-7786 My public PGP key is available at http://pgp.mit.edu |
From: Kai H. <Kai.Hartmann@Uni-Koeln.De> - 2004-08-13 08:05:05
|
Hi again. > I think a certan degree of unification is ok, even if it breaks some=20 > functions. The get...(At) methods should be uniform. I don't like the i= dea of=20 > having a lot of methods doing the same (like get and getAtom). I think = we=20 > should also keep names descriptive. So getSize is not good, since what = is the=20 > size of a molecule? I don't have a clear opinion about the last part. The size() and get()=20 methods would make it easier to run through container classes for cdk=20 newbies. I think for most containers it is clear what size() means -=20 even in *Atom*Container. On the other hand, I don't like the concept=20 TimTowTdi very much either. And the getXXXCount() naming convention is=20 okay as it is standardised for all container classes (get + classname +=20 Count). Kai > Stefan >=20 > Am Thursday 12 August 2004 16:42 schrieb Kai Hartmann: >=20 >>Hi all, >> >>now that a few changes are made to the API and the behaviour of methods >>(see clone and copy), I would like to suggest a check of the cdk method >>names and class names (core only) for consistency. >> >>1. One example: >>AtomContainer.getAtomAt(int) >>AtomContainer.getBondAt(int) >><-> >>SetOfAtomContainers.getAtom(int) >>SetOfMolecule.getMolecule(int) >> >>I always forget the "at" in the first two method names :-) >> >> >>2. Another one: >>RingSet <-> SetOfMolecules, SetOfAtomContainers >> >>But maybe that is intended (RingSet extends Vector, SetOfBla not). >> >> >>3. I would also prefer every class to have a "size" convenience method >>in addition to the getBlaCount, and maybe also a "get" (e.g. >>AtomContainer.get(int) in addition to getAtom). >> >>I know that changes to the core of cdk would break functionality of man= y >>existing projects. And I don't know what you think about deprecated >>methods. >> >>Comments are very much appreciated, >> >>Kai >=20 >=20 --=20 Kai Hartmann CUBIC - Cologne University BioInformatics Center Institute for Biochemistry Z=FClpicher Strasse 47 50674 Koeln Phone: +49-221-470-7719 Fax: +49-221-470-7786 |
From: E.L. W. <eg...@us...> - 2004-08-13 08:10:21
|
=2D----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Friday 13 August 2004 10:02, Kai Hartmann wrote: > > I think a certan degree of unification is ok, even if it breaks some > > functions. The get...(At) methods should be uniform. I don't like the > > idea of having a lot of methods doing the same (like get and getAtom). I > > think we should also keep names descriptive. So getSize is not good, > > since what is the size of a molecule? > > I don't have a clear opinion about the last part. The size() and get() > methods would make it easier to run through container classes for cdk > newbies. I think for most containers it is clear what size() means - > even in *Atom*Container.=20 Is it the number of atoms, or bonds, or electroncontiainers, or=20 atom+electroncontainers ... Egon =2D --=20 eg...@us... =2D --------------------------------------- CDK: http://cdk.sf.net/ JChemPaint: http://jchempaint.sf.net/ Jmol: http://www.jmol.org/ =2D----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (SunOS) iD8DBQFBHHdnd9R8I9Yza6YRArelAJ4pP4EnRXKAwJo1Hy6MT1hi1WqivQCeN7mu ckMk8cp7DXpu8pMCI6IEUjg=3D =3DVQOK =2D----END PGP SIGNATURE----- |