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.

Instructions:

• 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