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.
- Identify where significant speed-up may be possible.
- Identify where code can be written more transparently.
- 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?
- 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 |