SteinBlog

Reviewing the CDK VFLIB patch

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:

1. browse the code
2. mark up code you think is buggy
3. note missing unit tests
4. note missing JavaDoc
5. warn for subjected PMD warnings
6. optionally note other problems
7. 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.

Tagged with:

Categorised as: Open Science