Release 4.4 Code Reviews (Sandbox)

From IGSTK

Jump to: navigation, search

Contents

Release 4.4 Code Reviews

Note: This will be the last release of IGSTK under CVS. We are planning on moving to Git in the next release
Keep this in mind while doing code reviews. More details on moving to Git and reorganization of IGSTK source code directory can be found here:
IGSTK5.0

List of reviewers

Please fix the code when you do the review

  • SO: Sebastian Ordas
  • ZY: Ziv Yaniv
  • PC: Patrick Cheng
  • AE: Andinet Enquobahire
  • OG: Ozgur Guler

Code to be moved into main IGSTK

  1. igstkAscension3DGTracker
  2. Example/TrackerDataLogger
  3. TrackerController is bring updated to include support Ascension3DGTracker, need review
  4. PET Image reader classes
  5. Examples
    1. PETCTNeedleBiopsy ?

Code to be removed from sandbox

  1. igstkVideoFrameRepresentation
  2. igstkVideoFrameSpatialObject
  3. igstkVideoGrabber
  4. igstkVideoImager
  5. igstkVideoImagerTool

Code to be removed from main CVS

Other cleanup items

Code review table & assignment

Core igstk components:

Item Author Reviewer 1 Reviewed Code Fixed Reviewer 2 Reviewed Code Fixed Moved
igstkAscension3DGTracker.h Ascension AE Done - PC yes yes
igstkAscension3DGTracker.cxx id AE Done - PC yes yes
igstkAscension3DGTrackerTool.h id AE Done - PC yes yes
igstkAscension3DGTrackerTool.cxx id AE Done - PC yes yes
Testing/igstkAscension3DGTrackerTest.cxx id AE Done - PC yes yes
*
Examples\TrackerDataLogger\igstkTrackerDataLogger.h Michel Audette AE PC
Examples\TrackerDataLogger\igstkTrackerDataLogger.cxx id AE PC
Examples\TrackerDataLogger\igstkTrackerDataLoggerConfigurationFileReader.h id AE PC
Examples\TrackerDataLogger\igstkTrackerDataLoggerConfigurationFileReader.cxx id AE PC
Examples\TrackerDataLogger\igstkTrackerDataLoggerConfigurationXMLFileReader.h id AE PC
Examples\TrackerDataLogger\igstkTrackerDataLoggerConfigurationXMLFileReader.cxx id AE PC
Examples\TrackerDataLogger\igstkTrackerDataLoggerMainCommandLine.cxx id AE PC
Examples\TrackerDataLogger\igstkTrackerDataLoggerMainFLTK.cxx id AE PC
*
igstkPETImageReader.h Andinet PC yes yes SO yes yes
igstkPETImageReader.cxx Andinet PC yes yes SO yes yes
igstkPETImageSpatialObject.h Andinet PC yes yes SO yes yes
igstkPETImageSpatialObject.cxx Andinet PC yes yes SO yes yes
igstkPETImageSpatialObjectRepresentation.h Andinet PC yes yes SO yes yes
igstkPETImageSpatialObjectRepresentation.cxx Andinet PC yes yes SO yes yes
igstkPETImageReaderTest.cxx Andinet PC yes yes SO yes yes
igstkPETImageSpatialObjectTest.cxx Andinet PC yes yes SO yes yes
igstkPETImageSpatialObjectRepresentationTest.cxx Andinet PC yes yes SO yes yes

Specific code review comments & discussions (sorted by reviewer)

SO: Sebastian Ordas

General comments

  1. is "sudo make install" in sandbox working for you?
  2. which reslicing class to keep? I would go for Patrick's new one: igstkImageSliceRepresentationPlus (maybe with a another name, emphasizing that blending is now possible given two data sets) Most of current failing tests come from the old ImageSliceRepresentation class
  3. try running any application (e.g. PET-CT) and quit without doing anything: there's an itkLightObject attempt to delete .. complain that I cannot find where it comes from
  4. is InterventionalPlanSettings/IO going to replace previous much simpler PathIO stuff?

Class/File 1

  1. Comment 1
    • Response 1
    • Response 2
  2. Comment 2

Class/File 2

  1. Comment 1
    • Response 1
    • Response 2
  2. Comment 2

ZY: Ziv Yaniv

PC: Patrick Cheng

AE: Andinet Enquobahire

OG: Ozgur Guler

Code Review Check List

The following is the list of coding style issues that must be verified on every class and file during a code review.

Please also refer to the IGSTK Style Guide (PDF)

  • Filename must match class name
  • All files must have the copyright notice at the top
  • #define for class name in the headers
  • Brief class doxygen description (Must be a sentence terminated in a period ".")
  • namespace igstk
  • Complete class doxygen description
  • Constructor and Destructor must be protected if the class use SmartPointers
  • Operator= and Copy constructor must be declared private and not be implemented if the class use SmartPointers
  • No acronyms in class name or method names
  • no unnecessary headers #included
    • If possible, vtk classes should have a forward declaration instead of including their header.
  • Justify every public method
  • All member variables must be private
  • Typedefs that are not needed for the public API must be declard in the private/protected section as appropriate.
  • 100% code coverage (see dashboard)
  • All 'non-const' method must justify why they are not 'const'
  • In classes with StateMachine the method ProcessInputs() should only be called from Request methods.
  • All classes derived from itk::Object or vtkObject must have a PrintSelf method
  • All classes deriving from igstk::Object must have one (and only one) of the following macros
    • igstkStandardAbstractClassTraitsMacro
    • igstkStandardClassTraitsMacro
    • igstkStandardTemplatedAbstractClassTraitsMacro
    • igstkStandardTemplatedClassTraitsMacro
  • Any information that is printed or displayed has to be legible to human eyes
  • Maximum line length should be 78 chars
  • If the class has a state machine,
    • The state machine diagram needs to be exported by adding an entry to igstkStateMachineExportTest
    • State Machine diagram needs to be added to the doxygen documentation. To do this, \image with the name of the state machine diagram needs to be added to the detailed description of the class (documentation in the header) .
Retrieved from "http://public.kitware.com/IGSTKWIKI/index.php/Release_4.4_Code_Reviews_%28Sandbox%29"
Personal tools
TOOLBOX
LANGUAGES