Code Review

This project was reviewed by K. Krings, March 30, 2017.

Project information

First glance

The project looks mostly complete: documentation and one example script are available, unit tests or test scripts are however missing. The example script expects command line arguments and has no defaults; it should use input file(s) from our test data if possible.

The code compiles without errors or warnings in both release and debug mode.

Documentation

The Sphinx documentation is not build when running make html because there is no DOCS_DIR specified in CMakeLists.txt. Moreover, the documentation contains some syntax errors causing warnings:

  • Line 12: title underline is too short.

  • Lines 15 to 17: blank line and indentation are missing.

  • Lines 20 to 46: same as before plus missing space after *.

  • Lines 50 to 67: should be re-formatted as a bullet or description list.

The documentation gives a short introduction to what is calculated, describes the module’s output, and which parameters can be set. A more detailed explanation of the veto is given in the linked presentation. I think, it would make sense to copy some of the presentation’s content to the documentation and thus make it more detailed. Moreover, I would suggest to remove the reference to the personal software build, because the path could for example change at some point or even disappear. Instead, I would change the example in a way that it is using the specified input files by default.

At least StartingTrackVetoUtils.h could use some Doxygen documentation, which should be linked to the Sphinx documentation.

Source code

Code structure

The project’s code structure follows our guidelines.

Coding standards

  • StartingTrackVeto.cxx:36: parameter has no default value.

  • StartingTrackVetoUtils.cxx:65: I would give the OM iterator a more meaningful name because the for-loop spans over a lot of lines.

  • The header files include libraries that are only used in the implementations and should only be included there.

  • I have the feeling that the code is using a lot of raw pointers where they are actually not needed and could be replaced by references for example.

Readability

In general, the code is easy to understand, but I think the readability would definitely be improved by introducing more black lines to separate code blocks that belong together. I also encountered that the usage of tabs and spaces as well as the code indentation is not always consistent. The code should also be re-checked for missing or not necessary spaces before and after operators, respectively; for example StartingTrackVetoUtils.cxx:225.