It is my duty today to review a patch for some piece of CDK code again. I blogged about this process earlier.
This particular patch was submitted by Mark Rijnbeek in my team via the CDK patch tracker. I first go to the patch page, assign the patch to myself and set the patch status to “pending” to indicate that it is being worked on. It patches a piece of code which uses the VFLIB graph matching library to provide substructure searches with an Ullman and a VF2 algorithm.
Again, for my own records:
The executive summary of the reviewing task goes like:
- browse the code
- mark up code you think is buggy
- note missing unit tests
- note missing JavaDoc
- warn for subjected PMD warnings
- optionally note other problems
- optionally any other comment you have
And this is how it went – I’m leaving out things that were not applicable:
Browse the code and mark-up buggy parts
Egon had made an older version of this code available via GIT. I checked it out and looked at the code, which looked horrible because it was a 1:1 translation of a horrible looking C code. Clearly, a decent naming of the variables would greatly improve the code but I remember a statement that the translator himself could not make sense out of this, so the original author is to blame 
. I do not get the impression that this problem can be rectified quickly. In fact, it took Mark a few days to debug this code by adding a rich collection of debug messages. I’m not sure that this is how it should be. The code is essentially unreadable.
Note missing unit tests and javadoc
Mark and Rajarhsi supplied a number of unit tests for the code and they all pass. The code itself has javadoc and there are usage examples – very good.
Meanwhile, Egon, our CDK Uberworker, has posted the following:
Hi Rajarshi, Mark, I have had a look at the vflib branch, and note that the code is aimed at the standard module; like all new code, but for code in this module in particular, should adhere to CDK's 'stable' standards... (BTW, there is a Nightly at [0] which has been running on an older version of the patch) The below are some guidelines, please feel free to ask me or search the cdk-devel archives for the details. 1. clean JavaDoc You can use DocCheck to check that your clean has clean JavaDoc: ant -f javadoc.xml doccheck A common error is missing periods at the end of first sentences in the JavaDoc. The first sentence is important to get right, per JavaDoc standards. 2. no PMD warning (or with a good excuse) ant -f pmd.xml 3. unit test coverage Each module has a test suite MfooTests, which points to a Test class doing coverage testing... new unit tests classes must be added to this suite, MstandardTests for the vflib patch. The coverage testing class will then check that all new code is tested. I note missing tests of NodePair and State. Then these issues have been resolved, I'll look at the code/functionality itself.
Right – so that happens when you are not fast enough. And I stopped being fast enough long ago: So I guess I’ll just leave it where Egon is leaving it. So, guys, go and fix that stuff and then we’ll look at it again
And now I go for a run.
“I checked it out and looked at the code, which looked horrible because it was a 1:1 translation of a horrible looking C code. Clearly, a decent naming of the variables would greatly improve the code but I remember a statement that the translator himself could not make sense out of this, so the original author is to blame
. I do not get the impression that this problem can be rectified quickly. In fact, it took Mark a few days to debug this code by adding a rich collection of debug messages. I’m not sure that this is how it should be. The code is essentially unreadable.”
Chris, my point exactly:
http://depth-first.com/articles/2008/11/13/one-of-these-things-is-not-like-the-other
Rajarshi and I both made attempts to port the C code. The C code might make sense to a C programmer, but Java is really a completely different language with a totally different paradigm. Rajarshi made it to the end, but I gave up in despair. There was no way I wanted to be fighting with code like that for the next few years.
The reason I gave up on porting: unreadable code is unmaintainable and non-extendable code. It promotes bitrot, no matter how well-tested it is.
Then I discovered that the documentation on the VF algorithm is surprisingly easy to read. The algorithm lent itself very nicely to a simple, object-oriented implementation:
http://amalfi.dis.unina.it/graph/db/papers/vf-algorithm.pdf
This is why I created a Java-centric implementation of the VF algorithm from scratch for MX:
http://depth-first.com/articles/2008/11/13/one-of-these-things-is-not-like-the-other
Implementation:
http://github.com/rapodaca/mx/tree/00b9b4e8ee0627a8a4a965cebe510ecb50a5d6cd/src/com/metamolecular/mx/map
Main unit tests are here:
http://github.com/rapodaca/mx/blob/00b9b4e8ee0627a8a4a965cebe510ecb50a5d6cd/src/com/metamolecular/mx/test/DefaultQueryMapperTest.java
High-level documentation of the code is available here:
http://depth-first.com/articles/2008/11/17/substructure-search-from-scratch-in-java-part-1-the-atom-mapping-problem
It’s MIT licensed, so feel free to port if you’d like.
Rich, thanks so much for the great comment. We’ll definitely have a look at it. I will point it our to Mark, my OrChem developer. He uses Rajarshi’s VFLib code extensively but would certainly welcome some maintainable code. Cheers, Chris
Hi Chris,
I have implemented VFLib code from Rich (avoided reinventing the wheel) into SMSD for Substructure search. It works pretty well.
The code is very much in object oriented format and I have made necessary changes in the code to work with CDK.
Would be happy to share the code if someone is interested.
Asad
Hi Asad, thanks a lot for your comment and offer. We are currently sorting out with Rich a few glitches in his code. Please look at http://depth-first.com/articles/2009/06/16/if-the-wheel-doesnt-work-reinvent-it and the discussion therein. The patches resulting from this might be interesting for you. I’ll point Mark Rijnbeek in my group to your offer. Cheers, Chris
Make your life more easy take the loan and everything you want.
I will recommend not to wait until you earn big sum of money to buy different goods! You can just get the business loans or just consolidation loans and feel yourself fine