Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#550 SpanningTree/PathTools must be "CDK stable"

master
closed
nobody
9
2015-01-01
2007-10-21
Egon Willighagen
No

SpanningTree and PathTools are critical bits of code, and needed for the CDK atom typing. The code needs to be cleaned up and made 'CDK stable'.

Discussion

  • Rajarshi Guha
    Rajarshi Guha
    2007-10-25

    Logged In: YES
    user_id=349408
    Originator: NO

    Beyond code cleanup (such as moving Java 1.5 idioms as I committed) what remains to make it 'stable'. Unit tests are all passing

     
  • Logged In: YES
    user_id=25678
    Originator: YES

    (thanx for asking)

    Add missing JUnit tests [1]:
    SpanningTree: missing the expected test method: testClear
    SpanningTree: missing the expected test method: testGetPath_IAtomContainer_IAtom_IAtom
    SpanningTree: missing the expected test method: testIsDisconnected
    SpanningTree: missing the expected test method: testGetSpanningTree
    SpanningTree: missing the expected test method: testResetFlags_IAtomContainer
    SpanningTree: missing the expected test method: testGetBasicRings
    SpanningTree: missing the expected test method: testGetAllRings
    SpanningTree: missing the expected test method: testGetSpanningTreeSize
    PathTools: missing the expected test method: testGetInt2DColumnSum_arrayint
    PathTools: missing the expected test method: testResetFlags_IAtomContainer
    PathTools: missing the expected test method: testDepthFirstTargetSearch_IAtomContainer_IAtom_IAtom_IAtomContainer
    PathTools: missing the expected test method: testBreadthFirstSearch_IAtomContainer_Vector_IMolecule
    PathTools: missing the expected test method: testBreadthFirstSearch_IAtomContainer_Vector_IMolecule_int
    PathTools: missing the expected test method: testComputeFloydAPSP_arrayint
    PathTools: missing the expected test method: testComputeFloydAPSP_arraydouble
    PathTools: missing the expected test method: testFindClosestByBond_IAtomContainer_IAtom_int
    Fix all PMD warnings [2]:
    org.openscience.cdk.graph.PathTools 53 The class 'PathTools' has a Cyclomatic Complexity of 5 (Highest = 12).
    org.openscience.cdk.graph.PathTools 55 Variables that are final and static should be in all caps.
    org.openscience.cdk.graph.PathTools 82 Avoid variables with short names like C
    org.openscience.cdk.graph.PathTools 83 Avoid variables with short names like i
    org.openscience.cdk.graph.PathTools 84 Avoid variables with short names like j
    org.openscience.cdk.graph.PathTools 85 Avoid variables with short names like k
    org.openscience.cdk.graph.PathTools 86 Avoid variables with short names like n
    org.openscience.cdk.graph.PathTools 87 Avoid variables with short names like A
    org.openscience.cdk.graph.PathTools 120 Avoid variables with short names like C
    org.openscience.cdk.graph.PathTools 121 Avoid variables with short names like i
    org.openscience.cdk.graph.PathTools 122 Avoid variables with short names like j
    org.openscience.cdk.graph.PathTools 123 Avoid variables with short names like n
    org.openscience.cdk.graph.PathTools 124 Avoid variables with short names like A
    org.openscience.cdk.graph.PathTools 202 Avoid variables with short names like ac
    org.openscience.cdk.graph.PathTools 217 Avoid variables with short names like a
    org.openscience.cdk.graph.PathTools 217 Avoid variables with short names like ac
    org.openscience.cdk.graph.PathTools 219 Avoid variables with short names like v
    org.openscience.cdk.graph.PathTools 223 Avoid variables with short names like k
    org.openscience.cdk.graph.PathTools 249 Avoid variables with short names like ac
    org.openscience.cdk.graph.PathTools 249 The method 'breadthFirstSearch' has a Cyclomatic Complexity of 10.
    org.openscience.cdk.graph.PathTools 305 Avoid variables with short names like ac
    org.openscience.cdk.graph.PathTools 339 Avoid variables with short names like ac
    org.openscience.cdk.graph.PathTools 418 Avoid variables with short names like n
    org.openscience.cdk.graph.PathTools 441 The method 'getShortestPath' has a Cyclomatic Complexity of 12.
    org.openscience.cdk.graph.PathTools 441 The method getShortestPath() has an NPath complexity of 444
    org.openscience.cdk.graph.PathTools 453 Avoid variables with short names like S
    org.openscience.cdk.graph.PathTools 454 Avoid variables with short names like Q
    org.openscience.cdk.graph.PathTools 461 Avoid variables with short names like u
    org.openscience.cdk.graph.PathTools 486 Avoid variables with short names like u
    org.openscience.cdk.graph.SpanningTree 44 The class 'SpanningTree' has a Cyclomatic Complexity of 4 (Highest = 11).
    org.openscience.cdk.graph.SpanningTree 49 Avoid variables with short names like cb
    org.openscience.cdk.graph.SpanningTree 86 Avoid variables with short names like v1
    org.openscience.cdk.graph.SpanningTree 86 Avoid variables with short names like v2
    org.openscience.cdk.graph.SpanningTree 87 Avoid variables with short names like i
    org.openscience.cdk.graph.SpanningTree 88 Avoid variables with short names like j
    org.openscience.cdk.graph.SpanningTree 89 Avoid variables with short names like t
    org.openscience.cdk.graph.SpanningTree 108 Avoid variables with short names like V
    org.openscience.cdk.graph.SpanningTree 119 The method 'buildSpanningTree' has a Cyclomatic Complexity of 11.
    org.openscience.cdk.graph.SpanningTree 119 The method buildSpanningTree() has an NPath complexity of 360
    org.openscience.cdk.graph.SpanningTree 132 Avoid variables with short names like v1
    org.openscience.cdk.graph.SpanningTree 132 Avoid variables with short names like v2
    org.openscience.cdk.graph.SpanningTree 170 Avoid variables with short names like ac
    org.openscience.cdk.graph.SpanningTree 177 Avoid variables with short names like ac
    org.openscience.cdk.graph.SpanningTree 186 Avoid variables with short names like a1
    org.openscience.cdk.graph.SpanningTree 186 Avoid variables with short names like a2
    org.openscience.cdk.graph.SpanningTree 205 Avoid variables with short names like m
    org.openscience.cdk.graph.SpanningTree 265 Avoid variables with short names like m
    org.openscience.cdk.graph.SpanningTree 274 Avoid variables with short names like s
    org.openscience.cdk.graph.SpanningTree 313 Avoid variables with short names like i
    org.openscience.cdk.graph.SpanningTree 313 Avoid variables with short names like j
    org.openscience.cdk.graph.SpanningTree 313 The method 'combineRings' has a Cyclomatic Complexity of 11.
    org.openscience.cdk.graph.SpanningTree 314 Avoid variables with short names like c

    And fix JavaDoc issues. This also applies to all classes that these two classes depend on.

    1.http://cheminfo.informatics.indiana.edu/~rguha/code/java/nightly-1.0.x/test/result-standard.html
    2.http://cheminfo.informatics.indiana.edu/~rguha/code/java/nightly-1.0.x/pmd/standard.html

     
  • Rajarshi Guha
    Rajarshi Guha
    2007-10-25

    Logged In: YES
    user_id=349408
    Originator: NO

    OK - I see many PMD warnings. But i have to say, many of them are just plain irritating. i,j,k are common loop variables and I think most warning are just style issues which don't really add much - in a 5 line function why does it matter if the number of rows is the variable 'n' or 'nrow'?

     
  • Logged In: YES
    user_id=25678
    Originator: YES

    I agree to some extend, but want to comment that in code 'rowCount' is much easier to understand than 'n'. Likewise, 'atomIndex' is more readable than 'i'.

    A good example why this rule is useful is that 'ac' is used quite commonly... I just fixed a couple of bugs because uncloned atoms were looked up in the cloned atomcontainer, which will always fail... if the 'ac' had been named 'clonedContainer' this would have been much more apparent.

    Yes, it is more typing, but it also makes the code more readable and less prone to bugs.

     
  • John May
    John May
    2015-01-01

    Both these classes could be deprecated... we don't have replacements for everything in PathTools but the floydAPSP is slower than doing n shortest paths (i.e. using AllPairsShortestPaths).

     
  • John May
    John May
    2015-01-01

    • status: open --> closed