#180 new Bond() has two atoms??

Needs_Review
closed
nobody
cdk-1.2.x (46)
5
2012-10-28
2010-04-06
No

OK, the method is likely very rarely used, but I just ran into this bit of unit testing:

@Test
public void testBond() {
    IBond bond = new Bond();
    Assert.assertEquals(2, bond.getAtomCount());
    Assert.assertNull(bond.getAtom(0));
    Assert.assertNull(bond.getAtom(1));
    Assert.assertNull(bond.getOrder());
    Assert.assertEquals(CDKConstants.STEREO_BOND_NONE, bond.getStereo());
}

I'm pretty sure this thing does not have two atoms. I looked up the commit that made the assertion that this bond has two atoms, and traced is back to commit 35e8666d9573f3e19684c13cf6ab2c42471c28ed, which made this change:

  • org.openscience.cdk.interfaces.IAtom[] atoms = b.getAtoms();
  • assertEquals(2, atoms.length);
  • assertEquals(atomsToAdd[0], atoms[0]);
  • assertEquals(atomsToAdd[1], atoms[1]);
  • assertEquals(2, b.getAtomCount());
  • assertEquals(atomsToAdd[0], b.getAtom(0));
  • assertEquals(atomsToAdd[1], b.getAtom(1));

It asserted 2 before the patch too, but the first assert referred to the length of the array, not the atom count... easy mistake, I guess...

Discussion

  • Egon Willighagen

    Two patches applied. One to fix the unit test. Another that fixes the constructor and setAtom() method.

     
  • Egon Willighagen

    One more patch, to fix the same issue for NNBond.

     
  • Egon Willighagen

    Created a patch. There was a correlated problem with setAtom(IAtom, int) not updating the atom count, which is fixed by the patch too.

     
  • Egon Willighagen

    These four patches are rebased on top of the current master version.

     
  • Egon Willighagen

    This bug is also present in cdk-1.2.x. So, I've backported the patch, and will now attached 5 new patches.

     
  • Egon Willighagen

    Besides a bug fix, these patches also define a slightly different behavior (though I think intended):

    • new Bond() has zero atoms
    • setAtom() changes the atom count

    But, since it includes behavioral changes, it needs two reviews.

     
  • Christoph Steinbeck

    I looked at the patches and I agree that new Bond() should have zero atoms and return that accordingly upon request. I reviewed All of the patches with respect to implementing the changes above and I find them coherent and to the best of my knowledge complete. I suggest to apply those patches as they are.

     
  • Rajarshi Guha

    Rajarshi Guha - 2010-07-27

    Overall looks good. I think the new behavior is correct.

    One question - in setAtom, what is the upper bound of position? What happens if some specifies position = 10?

     
  • Egon Willighagen

    Good question... not sure... I guess it should dynamically increase the array internally...

    Thanx Christoph and Rajarshi... shall I commit them myself?

    Rajarshi, or will you signoff and commit? Or prefer to wait for a bit more feedback by others?

     
  • Rajarshi Guha

    Rajarshi Guha - 2010-07-27

    Well right now, IIRC, you will get an out-of-bounds exception. The value at which this occurs depends on how the bond was constructed. If you start the bond with the atom,atom constructor then the bond will always be a 2-center bond. If you create the bond with the IAtom[] constructor, you can set atos up to how many were specified in the constructor.

    I can sign of on this

     
  • Rajarshi Guha

    Rajarshi Guha - 2010-07-27

    Also, path 001 does not apply on latest cdk-1.,2.x, so I didn't try the others

     
  • Egon Willighagen

    Hahaha... that's me miscounting... that's the last patch of the current CDK 1.2.x :)

    Just start with 0002...

     
  • Rajarshi Guha

    Rajarshi Guha - 2010-07-27

    applied and pushed

     
  • Egon Willighagen

    I had to push another commit, with the same change made to the unit test for new Bond() and new NNBond():

    commit 6ef1fb1b36d0c5f8aab89be05039d4c7580178cb
    Author: Egon Willighagen egonw@users.sourceforge.net
    Date: Wed Jul 28 10:12:04 2010 +0200

    Updated the DebugBond unit test too now: new DebugBond() has zero atoms
    

    diff --git a/src/test/org/openscience/cdk/debug/DebugBondTest.java b/src/test/org/openscience/cdk/debug/DebugBondTest.java
    index b9e64d0..85acb33 100644
    --- a/src/test/org/openscience/cdk/debug/DebugBondTest.java
    +++ b/src/test/org/openscience/cdk/debug/DebugBondTest.java
    @@ -52,7 +52,7 @@ public class DebugBondTest extends AbstractBondTest {
    @Test
    public void testDebugBond() {
    IBond bond = new DebugBond();
    - Assert.assertEquals(2, bond.getAtomCount());
    + Assert.assertEquals(0, bond.getAtomCount());
    Assert.assertNull(bond.getAtom(0));
    Assert.assertNull(bond.getAtom(1));
    Assert.assertNull(bond.getOrder());

    I apparently overlooked adding that patch.

    Since the patch is trivial and applied to the other two constructors too, I took the liberty to skip peer review, so that I'll start releasing CDK 1.2.6 now.

     

Log in to post a comment.