Code Review Information

We are performing a code review of our offline software.

The primary goals:
  • Have new people understand (at a deep level) how each critical piece of analysis code works.
  • Find bugs this way, both syntax/typo bugs and the more difficult "incorrect intention" bugs.
  • Find fragility in the code.
  • Find anything else that might not be what is desired.
  • Characterize the level of documentation.
The secondary goals:
  • Identify where significant speed-up may be possible.
  • Identify where code can be written more transparently.
A few (not all!) of the things you should look for in the code:
  • Is the code sufficiently documented?
  • Any uninitialized variables?
  • Loops: start/end conditions as expected (e.g., "<" vs. "<=")? Nested loops not clobbering themselves (i vs. j)?
  • Are array bounds guaranteed to be respected based on the flow of the code?
  • Any memory leaks (e.g., "new" without "delete")?
  • Are the ART calls (getByLabel(), etc.) correct?
  • Is the behavior well-defined if some list or vector has an unusual number of entries (e.g., zero) or potential double entries (if possible)?
  • Anything hard-coded that shouldn't be? (input module names, detector cell sizes, ...)
  • Any compiler warnings?
As a first step, you should determine if the code is reviewable in its current state:
  • Is the code sufficiently commented throughout?
  • Is the intention of the algorithm sufficiently documented (in-code and/or externally, as appropriate)?
  • Is the code appropriately structured in terms of classes and functions (e.g., not one giant chunk of code in "reco" or "ana")?
  • Are there repeated blocks of code that should be replaced with a function call or loop?
  • Are variable names descriptive and reasonable for their scope (e.g., i is sometimes okay for a tiny loop but not for long or complex ones)?

Assignments and Instructions

The items included in this review have been chosen based on what is likely to generate the most fruit when weighed across all the goals listed above. We'll get to other pieces of code in due course.

  • Read the goals and tips above.
  • Go through any external documentation that is available for the code. You might ask the author/contact to tell you what's out there, what's out-dated, etc.
  • Determine if the code is reviewable (see above). If not, your first report should be a description of what is needed to make the code reviewable. If/when it is reviewable...
  • Read and understand the code.
  • Document your findings and thought processes as you go. Take notes (perhaps in your favorite word processor) and give comments for each file and each function that you review. These comments may sometimes be brief, but the more you write down and share, the more useful this will be.
  • Describe any issues that require action (see goals and tips above).
  • If the author/contact is helping you understand a piece of the code, ask that they also put their explanation directly into the code as C++ comments. Else, someone will be just as confused later.
  • If you add any comments to the code yourself, indicate that they are reviewer comments, in case you are actually misunderstanding something. (For example: "From reviewer Smith: this routine juggles five swords.")

The reviews of these items will all proceed at different paces, but they should all begin as soon as the assignment is made. Some items will have multiple reviewers, for example due to the extent of the code base. In such cases, the reviewers should work with the author/contact to make a plan of attack.

Contact Person: This is a starting contact person (not necessarily the original author). If a piece of code has been touched by too many hands, just email the specified mailing list as questions arise.

Review Status: A brief, free-form description of what's happening. Might say "assigned", "waiting for comments from author", "completed", etc.

DocDB Link: Take thorough notes, and post your findings and recommendations (whether already acted upon or not) to DocDB.

Package, Module, etc. Contact Person Reviewer(s) Review Status DocDB Link for Findings
CAFAna Backhouse Vahle assigned
ChannelInfo Paley
DiscreteTracker Backhouse
ElasticArms Niner Musser assigned
FuzzyKVertex Niner
Geometry (not the gdml) nova_offline
HoughTransform Baird Shanahan assigned
LEM Backhouse Toner assigned
MCCheater/BackTracker Lein
MuonRemove Sachdev Niner assigned
NuMISpillInfo Vahle undergoing rewrite
NumuEnergy Lein Baird assigned
Preselection Davies
QEEventFinder Raddatz
ReMId Raddatz Davies assigned
RecoJMShower Bian undergoing refactoring
Slicer/Slicer4D Baird Patterson completed; some action items still pending Full report here.
TrackFit/Kalman Raddatz Sachdev assigned
TrackFit/Cosmic Davies Tamsett assigned
Utilities/MathUtil Backhouse Patterson closed Full report here. Short code, so short review.
g4nova (except NovaG4Monopole*) nova_sim