#314 HIN reader handles aromaticring keywor

Needs_Review
closed
cdk-1.4.x (181)
5
2012-10-28
2011-02-12
Rajarshi Guha
No

Discussion

  • The test looks a bit weird, and testing that the carbons are aromatic twice?

    I will apply it anyway.

     
  • Rajarshi Guha
    Rajarshi Guha
    2011-02-13

    Well, the first portion tests whethre the appropriate atoms (in terms of index) have been flaged. The second part makes sure that they were the carbons

     
  • Should line 188-189 not be something like the following then:

    for (atom in mol) if (isAromatic(atom)) assert(notCarbon(atom))

    ?

     
  • Rajarshi Guha
    Rajarshi Guha
    2011-02-13

    No - only carbons are marked as aromatic in the test file, so it should be

    if (isAromatic(atom) asset(isCarbon(atom))

    or

    if (isCarbon(atom)) assert(isAromatic(atom))

     
  • Hahahaha... stupid mistake... I mean indeed:

    if (isAromatic(atom) asset(isCarbon(atom))

    But my point was that your other option (which matches what was in your patch) is already tested by the first asserts..