{"id":56,"date":"2008-09-01T19:13:34","date_gmt":"2008-09-01T17:13:34","guid":{"rendered":"http:\/\/www.steinbeck-molecular.de\/steinblog\/?p=56"},"modified":"2008-09-01T19:13:34","modified_gmt":"2008-09-01T17:13:34","slug":"creating-and-reviewing-patches-in-the-chemistry-development-kit-cdk","status":"publish","type":"post","link":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/2008\/09\/01\/creating-and-reviewing-patches-in-the-chemistry-development-kit-cdk\/","title":{"rendered":"Creating and Reviewing Patches in the Chemistry Development Kit (CDK)"},"content":{"rendered":"

In order to prevent major turbulences in the main source code development line of the Chemistry Development Kit (CDK), we decided a while ago to have separate branches in our subversion source code management system for each developer and each of his subprojects. Once a project has been finalized by a developer in her branch, she would then publish a patch in the CDK patch tracker system<\/a> and ask for it to be reviewed by posting to the CDK developers mailing list.<\/a> A CDK senior developer would the assign the patch to himself or another senior developer.<\/p>\n

I have just been assigned the task to review the recent Iterator\/Iterable patch for CDK and will protocol my task for reference reasons.The patch was published on the CDK patch tracker.<\/a><\/p>\n

The executive summary of the reviewing task goes like:<\/p>\n

    \n
  1. browse the code<\/li>\n
  2. mark up code you think is buggy<\/li>\n
  3. note missing unit tests<\/li>\n
  4. note missing JavaDoc<\/li>\n
  5. warn for subjected PMD warnings<\/li>\n
  6. optionally note other problems<\/li>\n
  7. optionally any other comment you have<\/li>\n<\/ol>\n

    So, let’s see how it went:<\/p>\n

    Browse the Code<\/strong><\/p>\n

    I got the gzipped archive with Egon’s patch and looked at the code. A large part of the changes involve<\/p>\n

    removing\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 public Iterator<IIsotope> isotopes() {<\/code>
    \nand adding\u00a0\u00a0\u00a0 public Iterable<IIsotope> isotopes() {
    \n<\/code>
    \nto enable things like<\/p>\n

    double overallCharge = 0.0
    \nfor (IAtom atom : molecule.atoms()) {
    \noverallCharge += atom.getCharge();
    \n}<\/code><\/p>\n

    In order to implement Iterable, one needs to have methods returning an Iterator, so a lot of code essentially implements those.<\/p>\n

    Remove: \u00a0\u00a0\u00a0\u00a0\u00a0 public java.util.Iterator atoms() {<\/code>
    \nand add: \u00a0\u00a0\u00a0\u00a0 public Iterable<IAtom> atoms() {
    \nlogger.debug(\"Getting atoms iterator\");
    \nreturn super.atoms();
    \n}<\/code><\/p>\n

    And then there is code actually using those iterators and all of these instances had to be adapted too (I’m just giving the patch syntax):<\/p>\n

    for(IReactionScheme rm : scheme.reactionSchemes()){
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for(Iterator<IAtomContainer> iter = getAllMolecules(rm, molSet).atomContainers(); iter.hasNext(); ){
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 IAtomContainer ac = iter.next();
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 boolean contain = false;
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for(Iterator<IAtomContainer> it2 = molSet.molecules();it2.hasNext();){
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 if(it2.next().equals(ac)){
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 contain = true;
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 break;
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 if(!contain)
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 molSet.addMolecule((IMolecule)(ac));
    \n-\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for (IAtomContainer ac : getAllMolecules(rm, molSet).atomContainers()) {
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 boolean contain = false;
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 for (IAtomContainer atomContainer : molSet.molecules()) {
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 if (atomContainer.equals(ac)) {
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 contain = true;
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 break;
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 if (!contain)
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 molSet.addMolecule((IMolecule) (ac));
    \n+\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0\u00a0 }
    \n<\/code><\/p>\n

    Overall, the patch affected 288 classes including test classes, with almost 2000 lines of code changed.<\/p>\n

    Mark up code you think is buggy<\/strong><\/p>\n

    Impossible to do for me for such a large bunch of changes, so one must rely here on the unit tests to work.<\/p>\n

    Note missing unit tests<\/strong><\/p>\n

    Egon had posted some notes about comparing failing and passing between unit tests<\/a> earlier but we also need an automatic check for unit test coverage. And yes, of course, there are limits to what such an automated coverage tool can do.<\/p>\n

    With regard for failing unit tests, the “iterable” branch did have anymore failures and errors than the head branch.<\/p>\n

    Note missing JavaDoc<\/strong><\/p>\n

    We’ve go DocCheck results on our CDK nightly pages but nothing tells you whether a patched method is missing neccessary JavaDoc. Presumably, we could “grep” the patches class names into a DocCheck input file and get customized info about it.<\/p>\n

    Warn for subjected PMD warnings<\/strong><\/p>\n

    PMD is a tool for checking code with respect to adherence to certain coding standards.Again, the CDK nightly page<\/a> contains all PMD reports on the CDK code, generated in nightly runs. The same can be achieved for each branch with a “ant -f pmd.xml”\u00a0 on your local copy of the branch.<\/p>\n

    Optionally note other problems<\/strong><\/p>\n

    I love optional things and tend to let them be optional<\/p>\n

    Optionally any other comment you have<\/strong><\/p>\n

    Dto.<\/p>\n

    So, overall I would like to conclude that according to the best of my knowledge, the Iterable patch should be safe and can be applied to the HEAD branch. <\/p>\n","protected":false},"excerpt":{"rendered":"

    In order to prevent major turbulences in the main source code development line of the Chemistry Development Kit (CDK), we decided a while ago to have separate branches in our subversion source code management system for each developer and each of his subprojects. Once a project has been finalized by a developer in her branch, […]<\/p>\n","protected":false},"author":2,"featured_media":0,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[23,14,27,18,29],"tags":[],"class_list":["post-56","post","type-post","status-publish","format-standard","hentry","category-chemistry-development-kit","category-life-of-chris","category-open-source","category-open-standards","category-scientific-culture"],"_links":{"self":[{"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/posts\/56"}],"collection":[{"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/users\/2"}],"replies":[{"embeddable":true,"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/comments?post=56"}],"version-history":[{"count":0,"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/posts\/56\/revisions"}],"wp:attachment":[{"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/media?parent=56"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/categories?post=56"},{"taxonomy":"post_tag","embeddable":true,"href":"http:\/\/www.steinbeck-molecular.de\/steinblog\/index.php\/wp-json\/wp\/v2\/tags?post=56"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}