Code Review
===========

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

Project information
-------------------

* URL: http://code.icecube.wisc.edu/svn/sandbox/kjero/StartingTrackVeto
* Repository Root: http://code.icecube.wisc.edu/svn
* Repository UUID: 16731396-06f5-0310-8873-f7f720988828
* Revision: 154499
* Last Changed Author: kjero
* Last Changed Rev: 153641
* Last Changed Date: 2017-03-01 04:24:58 +0100 (Wed, 01 Mar 2017)

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``.