# Code Review
Reviewer: Claudio Kopper <ckopper@icecube.aq>
This is a code review for LeptonInjector SVN r179409 found at
https://code.icecube.wisc.edu/svn/meta-projects/combo/branches/snobo/LeptonInjector/?p=179409 .
Guidelines used are from https://docs.icecube.aq/combo/trunk/general/code_reviews.html .
First glance:
* The project comes with an example script in resources/examples/inject_muons.py
* It includes a bare-bones README file for documentation in resources/doc/README
* There is currently no further documentation as part of the project.
* Several C++-based tests are included.
Source Code:
* The directory structure follows the guidelines laid out int the code review
guidelines. LeptonInjector does use a single public header `LeptonInjector.h`
* The project is laid out as a base class `LeptonInjectorBase` (drived
from `I3ConditionalModule` as expected) from which two classes for general
use are derived:
- `RangedLeptonInjector`
- `VolumeLeptonInjector`
- `MultiLeptonInjector` (which seems to combine "Ranged" and "Volume" modes.)
* Each injector I3Module has associated configuration objects derived from their
own common base class `BasicInjectionConfiguration` (a I3FrameObject):
- `RangedInjectionConfiguration`
- `VolumeInjectionConfiguration`
- `MinimalInjectionConfiguration` (associated with `MultiLeptonInjector`)
* There are several utility functions and modules collected in the same header
file. Everything is collected into a single namespace called `LeptonInjector`.
* Event properties are stored in `I3FrameObject`-derived objects of type
`RangedEventProperties` or `VolumeEventProperties` (both derived from
`BasicEventProperties`.)
* There seems to be functionality to store configuration objects in hdf5 files
(I note that they are also frame objects). This is in `private/LeptonInjector/h5write.cxx`
which currently includes some dead code in `/* */` blocks which should be
removed.
- The hdf5 code is only able to write `RangedInjectionConfiguration`
and `VolumeInjectionConfiguration` objects. No provision to store/write
`MinimalInjectionConfiguration` objects seems to exist. It is unclear why/if
these objects would not need to be serialized.
* I3/boost serialization code seems to exist in `private/LeptonInjector/serialization.cxx`.
Like with the hdf5 serializer, no mechanism exists to write `MinimalInjectionConfiguration`
objects. This file also contains a function to write boost-serializable
data to plain binary files. No provisions for reading data exist (although
I assume this would need to exist within LeptonWeighter).
I suggest adding documentation on what serialization scheme is to be used
in production and why.
* If there is no need to ever serialize `MinimalInjectionConfiguration` I would
suggest adding documentation describing why it is not necessary.
* The main code in `private/LeptonInjector/LeptonInjector.cxx` looks reasonable
except for some minor issues that SHOULD be fixed before production:
- line 907: LeptonInjectorBase objects are created as bare pointers but no
further memory management is performed. This will leak memory since objects
are never deleted. Trivially change to `unique_ptr` unless there is some major
concern with performance. Change type from `std::deque<LeptonInjectorBase*> generators`
to `std::deque<boost::unique_ptr<LeptonInjectorBase> > generators`. Also remove
the try/except block here. It serves no purpose once changed to shared pointers.
Since this only happens at module configuration time (and thus very likely only
once), it is not a severe issue, but should be fixed anyway.
* Python code: `LeptonInjector/python/__init__.py` contains exclusively commented
code. Either re-instate the code or remove it from `__init__.py` (potentially
adding a reference so that it can be re-extracted from version control history
in case future developers need it for reference).
Coding standards:
* Coding standards look reasonable, logging is included and variable naming is
reasonably clear.
Readability:
* I was mostly able to follow the code structure. Some comments on it are included
above.
Overall suggestions:
* Add some basic documentation as soon as possible.
* There might be some issues that could arise once this collection of modules
is used as part of a larger-scale simulation chain. Checking these has not
been part of this review.
* Fix the minor issues listed above.
* I recommend merging LeptonInjector into combo/trunk at this point so that
test are regularly run and development and test of a larger-scale simulation
pipeline including LeptonInjector can continue.
