[IGSTK-Developers] Code Revews CTImageReader - MRImageReader - LandmarkRegistration

[Luis Ibanez]

Hi  Andinet,

Here are code reviews for the
CTImageReader and the MRImageReader



CTImageReader


* Copy constructor and assigment operator are missing.
   They should be declared private and MUST NOT be implemented.

* Low code coverage, the Print() method was not being invoked
   in the test.

* Is lacking a state machine

* Is lacking verification for the Modality. That is, right now
   it can be used for loading an MR and the class doesn't
   report an error.


MRImageReader


* Copy constructor and assigment operator are missing.
   They should be declared private and MUST NOT be implemented.

* Low code coverage, the Print() method was not being invoked
   in the test.

* Is lacking a state machine

* Is lacking verification for the Modality. That is, right now
   it can be used for loading an MR and the class doesn't
   report an error.



LandmarkRegistration

* #include igstkMacros.h  was duplicated

* #include itkObject is not needed, it comes from the itk image

* #include itkMacros.h is not needed, it comes from the itk image

* #include itkEventObject.h is not needed, it comes from the itk image

* ImageType is fixed to be "unsigned char",
   this doesnt seem to be general enough. Do we really need an image
   type  ? or only an IndexType ?

* new lines missing between declaration of different types.
   Doxygen comments missing for types

* igstkTypeMacro() should use ::itk::Object when
   deriving from the itk::Object

* GetTransform() method should be removed.
   It is unsafe if not combined with a StateMachine

* RequestAddImageLandmarkPoint must receive a point
   passes as a const reference, not by copy.

* RequestAddTrackerLandmarkPoint must receive a point
   passes as a const reference, not by copy.

* In the .txx it is unecessary to have a #include
   for igstkEvents.h, it comes from the .h of this class already.

* There should be spaces around operators, and around
   arguments to functions and methods.

* Set at Get for LandmarkPointsContainer MUST be removed.
   They are unsafe, and destroy the StateMachine protocol.

* GetMacro for the Transform must be removed.
   It is unsafe, because it can be called before there is
   a valid transform. This should be controlled by a StateMachine.

* GetMacro TransformInitializer  MUST be removed. It is very unsafe.
   This internal ITK class should never be exposed in the API.

* In the test: it is dangerous to create events as raw pointers,
   pass them to AddObserver and then delete them at the end.
   It is better to declare their types with "typedef" and then
   use their constructors as arguments to AddObserver().


----------------------

Luis