#689 Fishy code in the UniversalIsomorphismTester

cdk-1.2.x
closed
Rajarshi Guha
None
9
2012-11-03
2008-07-26
Egon Willighagen
No

When cleaning up the API (List -> List<RMap> etc), I found a few fishy pieces of code, which apparently are occuring seldomly, as they should have caused ClassCastExceptions, I think. For example, this cannot be right, can it? :

public static List<RMap> getIsomorphAtomsMap(IAtomContainer g1, IAtomContainer g2) throws CDKException {
if (g1 instanceof IQueryAtomContainer)
throw new CDKException(
"The first IAtomContainer must not be an IQueryAtomContainer"
);

  List<RMap> list = checkSingleAtomCases(g1, g2);
  if (list == null) {
      return makeAtomsMapOfBondsMap(UniversalIsomorphismTester.getIsomorphMap(g1, g2), g1, g2);
  } else if (list.isEmpty()) {
      return null;
  } else {
      // FIXME: is this correct??
      return (List)list.get(0);
  }

}

The last return returns list.get(0), which must be an RMap, but is cast to a List???

Discussion

  • Rajarshi Guha
    Rajarshi Guha
    2008-08-10

    Logged In: YES
    user_id=349408
    Originator: NO

    I agree - looks fishy. Probably should read

    return list

    Looking at the function and its usage elesewhere in the CDK, I think the last else clause is never called since the method always gets a null list from checkSingleAtomCases() which I assume checks when a graph with a single vertex is used in a matching.

    Since the method is called from TemplateHandler, which will (I assume) never uses single atom templates, the dubious code never gets called.

    I think it's safe to just return the list in that clause (rather then the first elements

     
  • Logged In: YES
    user_id=25678
    Originator: YES

    Rajashi, please go ahead and adapt the code.