#48 Some solved threading issues

Needs_Revision
closed
nobody
None
5
2012-10-08
2009-06-08
Thorsten Meinl
No

This patch contains some fixes Jonathan and I made at the CDK workshop in Cambridge. Its mostly tagging classes/methods as thread safe (or not), fixing some thread-related issues, fixing some other issues and also some minor code cleanup.

Discussion

  • Thorsten Meinl
    Thorsten Meinl
    2009-06-08

     
    Attachments
  • Stefan Kuhn
    Stefan Kuhn
    2009-06-09

    I see no problems with that.

     
  • Rajarshi Guha
    Rajarshi Guha
    2009-06-10

    Patch looks OK, however I think it'd be a good idea to use annotations/tags rather than free text.

    So methods that are deemed threadsafe should have a Javadoc tag @threadsafe and those that are not would have @notthreadsafe

    This way a taglet can be written to generate a summary report, allowing devs to keep track of what has and has not been checked.

    Also I don't see the point of stuff such as

    • if (type == null) type = perceiveHalogens(atomContainer, atom);
    • if (type == null) type = perceiveCommonSalts(atomContainer, atom);
    • if (type == null) type = perceiveOrganometallicCenters(atomContainer, atom);
    • if (type == null) type = perceiveNobelGases(atomContainer, atom);
    • if (type == null) {
    • type = perceiveHalogens(atomContainer, atom);
    • }
    • if (type == null) {
    • type = perceiveCommonSalts(atomContainer, atom);
    • }
    • if (type == null) {
    • type = perceiveOrganometallicCenters(atomContainer, atom);
    • }
    • if (type == null) {
    • type = perceiveNobelGases(atomContainer, atom);
    • }

    The previous version (-) was pretty concise - why change it to the expanded version? I realize that this is generally a personal preference, but unless it is egregiously unformatted, I don't see a need to change it

     
  • Thorsten Meinl
    Thorsten Meinl
    2009-06-10

    I also favor the use of special Javadoc-tags.
    Regarding the formatting: that is certainly nothing that needs to be changed. These changes came from the auto-formatting we use in Eclipse for KNIME.

     
  • OK, the patch needs updating for the annotation tags introduced in:

    http://cdk.git.sourceforge.net/git/gitweb.cgi?p=cdk;a=commit;h=2e562a2434a7214ca06595d51e2ec467e1791be5

    instead of lines in the JavaDoc.

    I also like to see the use of final explained (not necessarily in the patch, but perhaps as separate patch), what role it has in thread safety... this could potentially lead to a unit test...

    I also note JavaDoc stubs like:

      • @param atomContainer
      • @return
      • @throws CDKException

    which interrupt the JavaDoc quality checking; please either: 1) remove such template JavaDoc, 2) complete that it.

    I also second Rajarshi's comment that changes like:

    • if (singleRing.getAtomCount() > maxRingSize) maxRingSize = singleRing.getAtomCount();
    • if (singleRing.getAtomCount() > maxRingSize) {
    • maxRingSize = singleRing.getAtomCount();
    • }

    are not necessarily needed, and actually make it much more work to review patches, as I now have to decide for the above if is is just style/layout or a secretly sneaking in code changes... I guess not, but I will have to check that one by one...

     
  • Rajarshi Guha
    Rajarshi Guha
    2010-10-16

    Any updates on this?