[IGSTK-Developers] Code Reviews : ImageReader : DICOMImageReader

[Luis Ibanez]

Andinet


Please find below my comments for the code review:

The ones with an (F) symbol were fixed right away,
as we agree on the tcon.


ImageReader

* the igstkTypeMacro()
   when having the itk::Object as superclass,
   should do "::itk::Object" instead of just "Object". (F)


* Methods that do not take arguments do not need the
   "void" specification. (F)

* #include for itkMacros.h is not necessary.
   It already comes from the Logger. (F)

* #include for itkObject.h is not necessary.
   It also comes already from the Logger (F).

* The content of the Doxygen \brief comment,
   must be a full sentence, terminated with a period ".".
   (F)

* Typedef for the internal ITK image (ImageType) must be
   protected, since the ITK layer must not be exposed. (F)

* Copy constructor and assignment operators
   should be declared but not implemented.
   They should be in the private section (F)

* Code Coverage is only 75%, The PrintSelf() method is not
   being invoked in the Test. (F)

* Code Coverage of the .h is only 50% because the GetITKImage
   is never called (it cannot be called.) Solution is to make
   this method abstract, and remove the New macro from this class.
   The test should accomodate by creating a class deriving from
   the ImageReader in order to test its methods. (F)


(with the changes above, the class now has 100% code coverage).

* The class lacks a StateMachine. (the infrastructure was added )(F)
   but still it remains to be programmed.


DICOMImageReader

* No need to include

    igstkMacros.h  (comes from the superclass)
    igstkStateMachine.h (comes from the superclass)
    itkLogger.h (it is not used at all)

    igstkEvents.h shouldnt be included in the .txx
              because it comes from the  .h already.

* The typedefs for the ImageSeriesReader must be protected.
   This shouldn't be exposed in the API.
   A matter of hiding the ITK layer. (F)

* The class must be made abstract (no NewMacro) and the
   test fixed for using a derived class created on the spot. (F)

* All Get() methods MUST be const

* Code coverage for the .h was only 76.92%

* Code coverage for the .txx was only 90.40%

* The Print() methods is not being invoked in the test,
   this will call the PrintSelf() method.

* Test are missing for error conditions
   (in particular the ITK eventual exception).
   We could test this by adding a bogus DICOM slice to the testing data.

* Copy constructor and assignment operators
   should be declared but not implemented.
   They should be in the private section (F)



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


   Luis