#689 Fishy code in the UniversalIsomorphismTester


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???


  • Rajarshi Guha

    Rajarshi Guha - 2008-08-10

    Logged In: YES
    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

  • Egon Willighagen

    Logged In: YES
    Originator: YES

    Rajashi, please go ahead and adapt the code.


Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.

No, thanks