More correct getMinimumBondOrder implementation, we don't cap at bond…

… order 4 and handle the corner cases. It's not clear whether the implicit hydrogens should be included but it makes sense this method is invariant depending on whether hydrogens are implicit/explicit. Also throw an exception if the atom is not in the container - whilst the silent failing state is nice sometimes it usually means something else has gone wrong. NoSuchAtomException fits better but is unfortunately checked so can't be used ATM.
cdk · May 7, 2017 · bfcc974 · bfcc974
1 parent 8f79f4c
commit bfcc974
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 11 deletions.
diff --git a/base/data/src/main/java/org/openscience/cdk/AtomContainer.java b/base/data/src/main/java/org/openscience/cdk/AtomContainer.java
@@ -27,6 +27,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Set;
 
 import org.openscience.cdk.interfaces.IAtom;
@@ -738,12 +739,23 @@ public Order getMaximumBondOrder(IAtom atom) {
      */
     @Override
     public Order getMinimumBondOrder(IAtom atom) {
-        IBond.Order min = IBond.Order.QUADRUPLE;
-        for (int i = 0; i < bondCount; i++) {
-            if (bonds[i].contains(atom) && bonds[i].getOrder().numeric() < min.numeric()) {
-                min = bonds[i].getOrder();
+        IBond.Order min = null;
+        for (IBond bond : bonds()) {
+            if (!bond.contains(atom))
+                continue;
+            if (min == null || bond.getOrder().numeric() < min.numeric()) {
+                min = bond.getOrder();
             }
         }
+        if (min == null) {
+            if (!contains(atom))
+                throw new NoSuchElementException("Atom does not belong to this container!");
+            if (atom.getImplicitHydrogenCount() != null &&
+                atom.getImplicitHydrogenCount() > 0)
+                min = Order.SINGLE;
+            else
+                min = Order.UNSET;
+        }
         return min;
     }
 

diff --git a/base/interfaces/src/main/java/org/openscience/cdk/interfaces/IAtomContainer.java b/base/interfaces/src/main/java/org/openscience/cdk/interfaces/IAtomContainer.java
@@ -440,11 +440,15 @@ public interface IAtomContainer extends IChemObject, IChemObjectListener {
     Order getMaximumBondOrder(IAtom atom);
 
     /**
-     * Returns the minimum bond order that this atom currently has in the context
-     * of this AtomContainer.
+     * Returns the minimum bond order that this atom currently has
+     * in the context of this AtomContainer. If the atom has no bonds
+     * but does have implicit hydrogens the minimum bond order is
+     * {@link IBond.Order#SINGLE}, otherwise the bond is unset
+     * {@link IBond.Order#UNSET}.
      *
      * @param atom The atom
-     * @return The minimim bond order that this atom currently has
+     * @return The minimum bond order that this atom currently has
+     * @throws java.util.NoSuchElementException atom does not belong to this container
      */
     Order getMinimumBondOrder(IAtom atom);
 

diff --git a/base/silent/src/main/java/org/openscience/cdk/silent/AtomContainer.java b/base/silent/src/main/java/org/openscience/cdk/silent/AtomContainer.java
@@ -27,6 +27,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.NoSuchElementException;
 import java.util.Set;
 
 import org.openscience.cdk.CDKConstants;
@@ -717,12 +718,23 @@ public Order getMaximumBondOrder(IAtom atom) {
      */
     @Override
     public Order getMinimumBondOrder(IAtom atom) {
-        IBond.Order min = IBond.Order.QUADRUPLE;
-        for (int i = 0; i < bondCount; i++) {
-            if (bonds[i].contains(atom) && bonds[i].getOrder().numeric() < min.numeric()) {
-                min = bonds[i].getOrder();
+        IBond.Order min = null;
+        for (IBond bond : bonds()) {
+            if (!bond.contains(atom))
+                continue;
+            if (min == null || bond.getOrder().numeric() < min.numeric()) {
+                min = bond.getOrder();
             }
         }
+        if (min == null) {
+            if (!contains(atom))
+                throw new NoSuchElementException("Atom does not belong to this container!");
+            if (atom.getImplicitHydrogenCount() != null &&
+                atom.getImplicitHydrogenCount() > 0)
+                min = Order.SINGLE;
+            else
+                min = Order.UNSET;
+        }
         return min;
     }
 

diff --git a/...st-interfaces/src/test/java/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java b/...st-interfaces/src/test/java/org/openscience/cdk/interfaces/AbstractAtomContainerTest.java
@@ -21,12 +21,14 @@
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
+import java.util.NoSuchElementException;
 
 import javax.vecmath.Point2d;
 
 import org.hamcrest.CoreMatchers;
 import org.junit.Assert;
 import org.junit.Test;
+import org.openscience.cdk.exception.NoSuchAtomException;
 import org.openscience.cdk.stereo.DoubleBondStereochemistry;
 import org.openscience.cdk.stereo.TetrahedralChirality;
 
@@ -1682,6 +1684,49 @@ public void testGetMinimumBondOrder_IAtom() {
         Assert.assertEquals(IBond.Order.SINGLE, acetone.getMinimumBondOrder(c3));
     }
 
+    @Test
+    public void testGetMinBondOrderHighBondOrder() {
+        IAtomContainer     container = (IAtomContainer) newChemObject();
+        IChemObjectBuilder builder   = container.getBuilder();
+        container.addAtom(builder.newAtom());
+        container.addAtom(builder.newAtom());
+        container.addBond(0, 1, IBond.Order.SEXTUPLE);
+        assertThat(container.getMinimumBondOrder(container.getAtom(0)),
+                   is(IBond.Order.SEXTUPLE));
+    }
+
+    @Test
+    public void testGetMinBondOrderNoBonds() {
+        IAtomContainer     container = (IAtomContainer) newChemObject();
+        IChemObjectBuilder builder   = container.getBuilder();
+        IAtom              atom      = builder.newAtom();
+        container.addAtom(atom);
+        assertThat(container.getMinimumBondOrder(atom),
+                   is(IBond.Order.UNSET));
+    }
+
+    @Test
+    public void testGetMinBondOrderImplH() {
+        IAtomContainer     container = (IAtomContainer) newChemObject();
+        IChemObjectBuilder builder   = container.getBuilder();
+        IAtom              a      = builder.newAtom();
+        a.setImplicitHydrogenCount(1);
+        container.addAtom(a);
+        assertThat(container.getMinimumBondOrder(a),
+                   is(IBond.Order.SINGLE));
+    }
+
+    @Test(expected = NoSuchElementException.class)
+    public void testGetMinBondOrderNoSuchAtom() {
+        IAtomContainer     container = (IAtomContainer) newChemObject();
+        IChemObjectBuilder builder   = container.getBuilder();
+        IAtom              a1      = builder.newAtom();
+        IAtom              a2      = builder.newAtom();
+        container.addAtom(a1);
+        assertThat(container.getMinimumBondOrder(a2),
+                   is(IBond.Order.UNSET));
+    }
+
     @Test
     public void testRemoveElectronContainer_int() {
         // acetone molecule