From IGSTK

Code reviews are typically done on a per iteration basis, so they will be organized per iteration here

Contents 1 Code Review Check List 2 Iteration 2 (2/3/05) 2.1 Kevin Gary 2.2 Hee Su's Review Notes: 2.2.1 Luis' Comments 2.3 Luis Ibanez 3 Iteration 3 (April 2005) 3.1 Classes to be reviewed 3.2 Hee-Su's Review Notes 3.3 Brian's Review Notes 3.4 David's Review Notes 3.5 Sohan's Review Notes 3.6 K2's Review Notes 4 Iteration 4 (6/17/05) 4.1 SA Code Review (a work in progress) 4.2 JJ Code Review 4.3 KG Code Review 4.4 HS Code Review 5 Iteration 5 5.1 DG Pre-Review Notes on Tracker and Communication Classes 5.2 JJ Code review 5.3 DG Code review 5.4 HS Code Review 5.5 Brian's Code Review 5.6 Luis' Code review 5.6.1 Serial Communication 5.7 Rick's Code Review 5.8 Patrick' Code review 5.8.1 Macro Expension Problem 6 Iteration 6

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.

• 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
• namespace igstk
• Complete class doxygen description
• Constructor/Destructor private/public
• No acronyms in class name or method names
• no unnecessary headers #included
• Justify every public method
• All member variables must be private
• 100% code coverage (see dashboard)
• All 'non-const' method must justify why they are not 'const'
• Any information that is printed or displayed has to be legible to human eyes

Iteration 2 (2/3/05)

Names are listed next to classes rom Luis' email. If you have the capacity or interest to review a class, please put your name next to the class. Our goal is to have these done by the t-con on 2/10/05 or before, and discuss. Thanks!

The specific classes to be reviewed are:

 * igstkSpatialObject_I2CR - KG, BB
 * igstkEllipsoidObject_I2CR - LI
 * igstkCylinderObject_I2CR - LI
 * igstkView_I2CR - BB
 * igstkView2D_I2CR - LI
 * igstkView3D_I2CR -LI 
 * igstkScene_I2CR - KG,BB
 * igstkObjectRepresentation_I2CR - KG, BB
 * igstkCylinderObjectRepresentation_I2CR - LI
 * igstkEllipsoidObjectRepresentation_I2CR - LI

Only the essential methods have been left public for being invoked by the users.

Kevin Gary

KG reviewed igstkSpatialObject, igstkCylinderObject, igstkObjectRepresentation, igstkCylinderObjectRepresentation, and igstkScene and put the review comments here: K2 iteration 2 comments.

Hee Su's Review Notes:

1. some files are using ITK as a program name: e.g. igstkScene, igstkObjectRepresentation, igstkCylinderObjectRepresentation, igstkCylinderObject, igstkEllipsoidObject, igstkEllipsoidObjectRepresentation
   Program: Insight Segmentation & Registration Toolkit
2. igstkView.cxx
   some duplicate headers included that were already included in igstkView.h
   vtkRenderWindow.h
   vtkRenderer.h
3. missing doxygen descriptions for some functions
4. igstkObjectRepresentation.h: Doxygen description for the ObjectRepresentation class is
   /** \class Object
   *
   * \brief Base class for all the igstk objects
   * \ingroup Object
   */
   I think this has to be changed like : class Object -> class ObjectRepresentation

Luis' Comments

• ITK copyright was replaced with IGSTK
• For the Doxygen comments missing in methods... we will need specifics.
• Duplicate headers from igstkView.cxx were removed
• Doxygen class description for igstkObjectRepresentation improved.

Luis Ibanez

• igstkEllipsoidObject
  • unused VTK headers removed
• igstkCylinderObject : OK
• igstkView2D was lacking Doxygen documentation for the class and the methods
• igstkView3D was lacking Doxygen documentation for the class and the methods
• igstkCylinderObjectRepresentation
  • Doxygen documentation was expanded
  • State Machine transitions were changed in order to honor only the first request for setting the spatial object.
• igstkEllipsoidObjectRepresentation
  • Doxygen documentation was expanded
  • State Machine transitions were changed in order to honor only the first request for setting the spatial object.

Iteration 3 (April 2005)

Classes to be reviewed

Name	Reviewer
igstkCylinderObject	HS
igstkCylinderObjectRepresentation	HS
igstkEllipsoidObject	HS
igstkEllipsoidObjectRepresentation	HS
igstkEvents	K2, SR
igstkMacros	HS, SR
igstkObjectRepresentation	K2, SR
igstkPulseGenerator	K2, SR
igstkScene	K2, SR
igstkSpatialObject	K2
igstkStateMachine	K2
igstkTimeStamp	K2
igstkTracker	BB
igstkTrackerPort	BB
igstkTrackerTool	BB
igstkTransform	BB
igstkTransformModifiedEvent	BB
igstkView	K2

Hee-Su's Review Notes

• igstkCylinderObject
  • The destructor is not virtual
• igstkCylinderObjectRepresentation
  • Constructor & destructor are in public.
  • The destructor is not virtual
  • PrintSelf() in public
  • CreateActors() in public
• igstkEllipsoidObject
  • The destructor is not virtual
• igstkEllipsoidObjectRepresentation : OK
• igstkMacros : OK

Brian's Review Notes

All of the standard check-list items were confirmed to the best of my review. Below are comments/observations I made as I went through the code.

• IGSTKTracker:

1. This may have already been discussed. There are cases that are not handled in GetToolTransform, SetToolTransform, and AttachObjectToTrackerTool. Should the "else" be added. Doesn't seem like the state machine handles these potential exceptions.

2.Probably should not be using literals in the code. Can we exchange this for a const or a configurable?

"m_PulseGenerator->RequestSetFrequency( 30 )" (Seems like someone already noted this by sayingthat 30 is not the correct number)

• IGSTKTrackerPort and IGSTKTrackerTool:

1. Do these really need a state machine? I saw justification for one potential exception-handling case. Is it overkill.

• IGSTKTransform:

1. Should this literal be removed. Sure this has to do with the matrix size. "outmatrix.SetElement(i,3,m_Translation[i]);"

2. Same comment as (1) but for the nested loops. I guess this is safe if this is never slated to change, otherwise it would be better practice to make it a const var or a configurable field.

3. Another literal, m_Translation.Fill( 0.0 );

4. This method is not intuitive to me:

void Transform::SetToIdentity( TimePeriodType millisecondsToExpiration ) m_TimeStamp.SetStartTimeNowAndExpireAfter( millisecondsToExpiration ); m_Translation.Fill( 0.0 ); m_Rotation.SetIdentity(); m_Error = itk::NumericTraits< ErrorType >::min();

Maybe my ignorance, this is a reminder for me to get smarter about what this does?

• IgstkPulseGenerator:

1. I'm a broken record (literals). "double PulseGenerator::m_MaximumFrequency = 10000; // 10 KHz" "m_Period = 1.0 / m_Frequency;"

• General:

1. Looking at Hee Su's comments, I guess we are promoting "all" virtual destructors. I'm a bit rusty in my C++, but it used to be that virtual destructors were reserved for pure virtual classes. If I am totally off-base, hit me upon the head.

David's Review Notes

• igstkTracker

Copyright, style, typedefs all check out. All member variables are private. Doxygen documentation is complete.

Methods:

Initialize() method takes a file as an argument. Should there be set/get FileName methods instead?

AttachObjectToTrackerTool(): Connecting tracker tools to spatial objects is great, but we also need easy methods of connecting the Transform to the ITK pipeline for use in e.g. extracting image slices that correspond to the tool position.

All methods are virtual, perhaps this is not good with respect to protected methods that are intertwined with the state machine.:

Code coverage is 75%, because 1) failure states aren't tested 2) attachment to spatial object not tested 3) closing of tracker is only tested from active tools state, not from any of the other states

• igstkTrackerTool

Indentation needs minor fixes.

There are two "public" sections and two "private" sections, where there could be only one of each

• igstkTrackerPort

Indentation needs minor fixes fixing.

The GetTool() method is not fully covered by testing, but all other methods are.

• General questions:

Should copy and assignment constructors be private, like in ITK and VTK?

It is good style for all destructors to be virtual.

Sohan's Review Notes

• igstkEvents.h

1. This file needs to be included in the CMakeLists.txt to get listed under the header files.

• igstkObjectRepresentation.h

1. I prefer all "friend" declarations right at the top of all definitions.
2. Question: Do we want to declare all parameters of all "Set" methods to be "Const"?
3. Question: Do we want to segregate "itk", "vtk" and "igstk" includes?
4. Question: Do we want to have "PrintSelf" in all IGSTK classes?

• igstkPulseGenerator.cxx

1. If dependence on FLTK may be avoided using IGSTK timers, that would be great.

• igstkScene.h/igstkScene.cxx

1. to be included in the CMakeLists.txt to get listed under the header files. I am wondering how the whole thing is getting compiled.

• igstkMacros.h

1. Common to all code: We need to remember that the igstkStateMachineMacro has "private" declaration for StateMachine part, but leaves out with a "public" declaration. So even if igstkStateMachineMacro() is defined in a "private" part of a code, the following declaration are "public".

K2's Review Notes

• igstkEvents
• igstkObjectRepresentation

1. Put the declaration of class member variables together.

• igstkSpatialObject

1. Would think it better to put igstkStateMachineMacro in the private area. Even though the macro takes care of this itself, the declaration here is misleading and might cause a problem if the macro is ever changed.
2. Should the const declaration of NoAction appearing in the middle of the setup of the state machines go in igstkStateMachine? igstkMacros?
3. I notice a difference in how null inputs to the state machine are handled in igstkObjectRepresentation and igstkSpatialObject. Handling null inputs in two different state machines is fine of course, but it is not clear to me why ObjectRepresentation would not perform an action (NoAction) while SpatialObjects would invoke an action ReportInvalidRequest. Should there be a set policy for dealing with null inputs?

• igstkStateMachine

1. Put typedef for ostream with rest of the typedefs at the top. Should this be in a globals file of some sort?
2. I would favor renaming ExportDescription to ExportToDot for extensibility reasons. We will want to add an ExportToXMI soon.
3. AddTransition does not check whether the state machine is ready to run. I would assume we would not add transitions to a running state machine?
4. Question - why did we not use a state machine to govern the state machine's states? Too confusing? To wasteful of memory? I ask because every state machine has a distinct set of (linear) states it transitions through.

• igstkPulseGenerator

1. if tests on frequency values, though I do not see an apparent way to get rid of these ifs
2. FIXME on using stderr. Other classes in IGSTK also are using cerr.
3. Not really a code comment - but the whole nature of the pulse generation used in conjunction with the observer model still makes me worry about unpredictable results, especially if there are requests sent across the layers of our system.

Iteration 4 (6/17/05)

Iteration 4 is tagged in main and Sandbox as Release-Iteration-4-Sandbox-Phase. David Gobbi posted a list of files to review below. Our goal is to review the week of June 20, so please sign up and complete reviews quickly. The code review checklist is available below, and based on our discussion on reviews for Iteration 3, there are several other best practices to check for which can be found under Iteration 3 notes above.

Name	Reviewer
igstkTracker	BB/JJ
igstkAuroraTracker	BB
igstkAuroraTool	BB/HS
igstkNDICommandInterpreter	HS
igstkSerialCommunication	KG/JJ
igstkSerialCommunicationForLinux	KG
igstkSerialCommunicationForWindows	KG
igstkVTKLoggerOutput	SA/JJ
igstkFLTKTextLogOutput	SA/JJ
igstkFLTKTextBufferLogOutput	SA/JJ

Code Review comments:

SA Code Review (a work in progress)

igstkVTKLoggerOutput.h

• Lines longer than 80 chars
• Space missing between each "#include" and its associated "<filename.h>"
• ...

JJ Code Review

igstkSerialCommunication.h

• Too many extra blank lines between groups (typedef)
• Macros should have one comment line per macro
• Some code is commented out
• What is the guideline for spaces: 'mytemplate< 3 >' v.s. 'mytemplate<3>'
• Missing space after /**
• Some comments have too many spaces in them
• m_pCommand is not a valid name should be m_PCommand or m_Command same goes for m_pLogger

igstkSerialCommunication.cxx

• Constructor initialization should be done in the constructor not after ':'
• Some extra blank lines (make the code hard to read)
• Are we respecting ITK's style guide for the indentation of the braces?
• Some of the indentation is 4 spaces instead of 2

igstkTracker.h

• Comments should have '*' at the beginning of each line
• Comments should start with /** and not /*
• Some code is commented out and should probably be removed if not used

igstkTracker.cxx

• The code has no comment (is this a requirement in the .cxx file ?)
• Some code is commented out

igstkVTKLoggerOutput.h

• Missing space between #include and the include file
• is "OSSystemObjects" a well defined group?
• "LoggerType" should be "LoggerPointerType"
• Some code is commented out and should probably be removed if not used
• braces indentation is wrong (not respecting ITK standard)

igstkVTKLoggerOutput.cxx

• Missing space between #include and the include file
• Do we need vtkCxxRevisionMacro(VTKLoggerOutput, "$Revision: 1.11 $") and vtkStandardNewMacro(VTKLoggerOutput);?
• Braces indentation is wrong (not respecting ITK standard)
• Some functions are not commented
• Some code is commented out and should probably be removed if not used

igstkFLTKTextLogOutput.h

• Missing space between #include and the include file
• Comments should have '*' at the beginning of each line
• Comments should start with Upper case (to my mind)
• Some comments are missing (typedefs)

igstkFLTKTextLogOutput.cxx

• Missing space between #include and the include file
• Some code is commented out and should probably be removed if not used
• Braces indentation is wrong (not respecting ITK standard)

igstkFLTKTextBufferLogOutput.h

• Missing space between #include and the include file
• Comments should have '*' at the beginning of each line
• Some comments are missing (typedefs)
• Comments should start with Upper case (to my mind)

igstkFLTKTextBufferLogOutput.cxx

• Missing space between #include and the include file
• Some code is commented out and should probably be removed if not used
• Braces indentation is wrong (not respecting ITK standard)

KG Code Review

igstkSerialCommunication.h

• echo JJ's comments on spaces
• Constructor and Destructor protected instead of private/public. May be good reason for this but should be justified if it deviates from our best practices.
• Lack of Doxygen complete class description

igstkSerialCommunication.cxx

• Thought we were naming public methods that invoke the state machine as "Request...", e.g. OpenCommunication --> RequestOpenCommunication? Perhaps I remember incorrectly, I do not see it in the best practices above.
• These are more questions than comments regarding method OpenCommunication, and may be because I do not understand fully how the changes in the StateMachine will effect the look of this implementation:

1. It goes through multiple state transitions within a single method. Perhaps this is not a problem for a convenience-style method, but what happens if it returns "false" from the middle of the method invocation? What will be the state?
2. The technique of relying on setting the value of a protected member variable in the subclass' method implementation (e.g. SerialCommunicationForLinux::OpenPortProcessing) and then testing the variable's value back in OpenCommunication does not seem safe to me. There is no compiler assurance the variable will be set and set properly.
3. The if test itself in OpenCommunication could be eliminated in favor of success/failure states

igstkSerialCommunicationForLinux.h

• Doxygen complete class documentation

igstkSerialCommunicationForLinux.cxx

• See above questions on igstkSerialCommunication.cxx
• Confused as to why the typedefs for baud rate defined in igstkSerialCommunication.h are not applied here, and instead straight ints and assignment are used.
• Several ifdef's for specific platforms, do we need if this will only be deployed on linux?

igstkSerialCommunicationForWindows.h

• Doxygen complete class documentation

igstkSerialCommunicationForWindows.cxx

• Similar comment about baud rate, also bits, parity, port numbers

HS Code Review

igstkNDICommandInterpreter.h

• too many #define
• methods defined in a header file occupied 2 lines
• question: /*! Is this a doxygen style? Some comments start with /*!.

igstkNDICommandInterpreter.cxx

• There are /**...**/ between functions without any comment.
• very short parameter/variable names such as 'cp', 'n'.
• a space is needed between the return type and method name.
  • some signatures are like this: char *NDICommandInterpreter::HexEncode(char *cp, const void *data, int n)
• CalcCRC16() is defined as a macro (#define). Why isn't this a normal method?
• Some codes were commented out.

igstkAuroraTool.h

• There is no copyright notice.
• Constructor & destructor are in public.
• Doxygen complete class documentation.

igstkAuroraTool.cxx

• Braces indentation is wrong (not respecting ITK standard)
• The constructor is not commented.

Iteration 5

We would like to complete Iteration 5 code reviews in a timely fashion so the project can transition to whatever process revisions are agreed upon at the 7/28 tcon. Below is the code review table. Per David's suggestion, the iteration 4 table above was copied to here so reviewers can check that their iteration 4 comments were addressed (since we did not tag the main codeline for iteration 4). Also, per Luis' suggestion, a requirements column has been so requirements that have been addressed in this iteration can be listed here to assist reviewers with validation. Please add to this list whatever files you have checked in during iteration 5 that require review.

Name	Reviewers	Reviewed	Code Fixed	Requirement
igstkTracker	BB/JJ	Y	Y	See R2_I5_REQ
igstkAuroraTracker	BB/LI	Y	Y	See R2_I5_REQ
igstkAuroraTool	BB/HS	Y	Y
igstkNDICommandInterpreter	HS/LI	Y	Y
igstkSerialCommunication	RA/JJ	Y	Y	See R2_I5_REQ
igstkSerialCommunicationForLinux	RA/LI	Y	Y	See R2_I5_REQ
igstkSerialCommunicationForWindows	RA/LI	Y	Y	See R2_I5_REQ
igstkVTKLoggerOutput	SA/JJ	Y	Y
igstkFLTKTextLogOutput	SA/JJ	Y	Y
igstkFLTKTextBufferLogOutput	SA/JJ	Y	Y
igstkMeshObject	DG/HS	Y	Y
igstkMeshObjectRepresentation	DG/HS	Y	Y
igstkTubeObject	DG/HS	Y	Y
igstkTubeObjectRepresentation	DG/HS	Y	Y

Code Review comments:

DG Pre-Review Notes on Tracker and Communication Classes

• I5 Tracker Requirements that are only partially completed:
  • Support for both Polaris and Aurora is provided through the igstkAuroraTracker class, whereas Polaris support should be provided though an igstkPolarisTracker class instead
  • igstkTracker::SetWorldTransform() needs to be renamed to igstkTracker::SetPatientTransform(), since we agreed to call our surgical tracker coordinate system "patient coordinates"

• I5 Communication Requirements that are only partially completed:
  • Non-canonical vs. canonical communication (i.e. fixed length data records vs. data records that are terminated by a special character)

Also, some of the I4 code review comments have not yet been addressed due to time constraints, I will go through those comments here so that me and HS can deal with them when the freeze is over:

• igstkSerialCommunication
  • Every method and macro needs a doxygen comment, this is not complete
• igstkTracker
  • SetWorldTransform and SetReferenceTool code is still commented out, it needs to be uncommented and tested, and WorldTransform needs to be renamed to PatientTransform
  • There is still a lack of comments in the code
• igstkNDICommandInterpreter
  • Some code is still commented out, and can't be uncommented and tested until certain SerialCommunication features have been implemented
• igstkAuroraTool
  • Still lacks copyright at top of file

JJ Code review

• igstkSerialCommunication.h

- Missing * for mutiple lines comments.

• igstkSerialCommunication.cxx

- Initialization of internal variables should be done in the constructor not after SerialCommunication() : (except for the state machine) - Comments are still missing

• igstkTracker.h

- Not changed since last code review

• igstkTracker.cxx

- Not changed since last code review

• igstkFLTKTextBufferLogOutput.h

- Copyright header is wrong

• igstkFLTKTextBufferLogOutput.cxx

- Copyright header is wrong - Flush() function doesn't do anything and do not print any messages when called (or documented why it's not implemented)

• igstkFLTKTextLogOutput.h

- Copyright header is wrong

• igstkFLTKTextLogOutput.cxx

- Copyright header is wrong - Flush() function doesn't do anything and do not print any messages when called (or documented why it's not implemented)

• igstkVTKLoggerOutput.h

- Copyright header is wrong - Private variable Logger should be m_Logger - virtual ~VTKLoggerOutput() {} is missing a semicolon after {} - virtual void OverrideVTKWindow() : bad indent for the {} - Any reason why itkLogger is not a smart pointer?

• igstkVTKLoggerOutput.cxx

- Copyright header is wrong

DG Code review

• igstkMeshObject
  • class description is a copy of the CylinderObject description
  • several typedefs should have comments to help people understand the code:
    • itk::DefaultDynamicMeshTraits: say what the parameters mean
    • itk::Mesh: say what the parameters mean
    • CellTraits, PointType, CellInterface: describe what these are
  • GetPoints() and GetCells() should only be const if they return const containers

• igstkMeshObjectRepresentation
  • constructor/destructor should be protected, not public
  • the code is not yet finished, but it looks like the most important parts are done
  • the test should display the mesh, instead of just connecting the pieces together

• igstkTubeObject
  • class description is a copy of the CylinderObject description
  • the typedef for itk::TubeSpatialObject<3> should have a short comment to describe the itkTubeSpatialObject
  • there is no method for changing the point positions?
  • since MeshObject has "GetPoints", shouldn't TubeObject also have a GetPoints() method?
  • the code for GetPoint(i) is inefficient, it is an O(n) method where a O(1) method could be used: why not just return points[i] since points is a vector?

• igstkTubeObjectRepresentation
  • better class description needed
  • constructor/destructor should be protected, not public

HS Code Review

• AuroraTool.h
  • There's no copyright notice.
  • What's standard for #define classname?
    • #define __igstk_Classname_h_
    • #define __igstkClassname_h_
    • #define __Classname_h_
  • Needs more doxygen description for the class.
  • Needs doxygen comments for some methods and typedefs.
• AuroraTool.cxx
  • Needs doxygen comments for the constructor.
• NDICommandInterpreter.h
  • methods defined in a header file occupied 2 lines (indentation seems wrong)
  • a space is needed between the return type and method name.
    • some signatures are like this: char *NDICommandInterpreter::HexEncode(char *cp, const void *data, int n)
• NDICommandInterpreter.cxx
  • Needs comments for each method.
  • a space is needed between the return type and method name.
    • some signatures are like this: char *NDICommandInterpreter::HexEncode(char *cp, const void *data, int n)
  • Some codes were commented out.
• MeshObject
  • David's suggestions were already reflected.
• MeshObjectRepresentation.h
  • Looks good
• MeshObjectRepresentation.cxx
  • Constructor and SetMeshObject() have a wrong comment.
    • // We create the ellipse spatial object -> mesh spatial object
  • SetMeshObject's comment is copied from CylinderObjectRepresentation.
• TubeObject.h
  • Looks good
• TubeObject.cxx
  • Constructor has a wrong comment. (saying ellipse)
• TubeObjectRepresentation.h
  • class doxygen description has: (missing 'of' between representation and a Tube object)
    • \brief This class implements the representation a Tube object.
• TubeObjectRepresentation.cxx
  • Constructor has a wrong comment saying ellipse.
  • RequestSetTubeObject()'s comment : Tubeal -> Tubal
  • SetTubeObject()'s comment says about cylindrical and ellipse spatial object.
  • UpdateRepresentation needs to be implemented.

Brian's Code Review

igstkTracker.cxx

• Remove commented code from "void Tracker::GetToolTransform"

• Several functions are commented out, should we remove them?
  • SetToolTransform
  • AttachObjectToTrackerTool
  • AddPort( TrackerPortType * port )
  • GetWorldTransform()
  • SetToolCalibrationTransform(
  • SetCommunication

• More commented code to remove
  • // m_ApplyingReferenceTool = false;

igstkAuroraTracker.h

• DOxygen comments need to be added throughout

• Remove this commented code
  • // TR_SWITCH1_IS_ON = 0x0010,
  • // various buttons/switches on tool
  • // TR_SWITCH2_IS_ON = 0x0020,
  • // TR_SWITCH3_IS_ON = 0x0040

• It is generally not a good practice to embed classes, should the NDIConfigurationReader class be embedded in igstkAuroraTracker be a class of it's own??

• • class NDIConfigurationReader : public itk::XMLReaderBase
  • {
  • virtual int CanReadFile(const char* name);
  • virtual void StartElement(const char * name,const char
  • **atts);
  • virtual void EndElement(const char *name);
  • virtual void CharacterDataHandler(const char *inData, int ** inLength);
  • };

igstkAuroraTracker.c

• We should either define the literals as constants, or there should be comments to explain the values, a couple of instances listed below

• • // initialize transformations to identity
  • for (int port = 0; port < NDI_NUMBER_OF_PORTS; port++)
  • {
  • transform8[port][0] = 1.0;
  • transform8[port][1] = 0.0;
  • transform8[port][2] = 0.0;
  • transform8[port][3] = 0.0;
  • transform8[port][4] = 0.0;
  • transform8[port][5] = 0.0;
  • transform8[port][6] = 0.0;
  • transform8[port][7] = 0.0;
  • }

• • TimePeriodType validityTime = 100.0;

• Commented code should be removed as shown below
  • // if (port_status & CommandInterpreterType::NDI_SWITCH_1_ON) { flags |= TR_SWITCH1_IS_ON; }
  • // if (port_status & CommandInterpreterType::NDI_SWITCH_2_ON) { flags |= TR_SWITCH2_IS_ON; }

igstkAuroraTool (h,cxx)

• There is not much code here (smile)
• Check to make sure DOxygen comments are complete, seemed a bit thin

Luis' Code review

Serial Communication

• Full Doxygen comment was missing. A first paragraph was added as part of this review.
• igstkTypeMacro() was using "Object" as the base class. It was fixed: replaced with "Communication"
• Doxygen is not expanding the IGSTK macros. Changes were made in Utilities/doxyfile.in in order to trigger the expansion of the IGSTK macros.
• Get methods are not declared "const"
• Most of the Set/Get methods should use the Set/Get Macros. Right now, they are not.

Rick's Code Review

igstkSerialCommunication.cxx

• 250: LE8C (Line exceeds 80 characters).
• 254: LE8C.
• 332: No error checking on the setting of timeout milliseconds. We should check for a positive number within a range.
• 361: No error checking on the setting of sleep milliseconds. We should check for a positive number within a range.
• 371: No error checking on the numberofBytes to write. We should check for a positive number.
• 380: What happens if m_recording is not set? How will the user know nothing has been written?
• 400: No error checking on the numberOfBytes to read. We should check for a positive number.
• 459: PrintSelf() does not report on many instance variables including:
  • SleepPeriod, InputData, OutputData, BytesToWrite, BytesToRead, Recording …

igstkSerialCommunicationForLinux.cxx

• 51: LE8C.
• 114: LE8C.
• 232: LE8C.
• 252: LE8C.
• 266: LE8C.
• 277: LE8C.
• 376: PrintSelf() does not report on Porthandle and SaveermIOs.

igstkSerialCommunicationForWindows.cxx

• 349: PrintSelf() does not report on PortHandle, SaveTimeout, and SaveDCB.

Patrick' Code review

Macro Expension Problem

• Current Doxygen script does not generate docs for Sandbox - fixed
• igstkNewMacro() expension incorrect - fixed
• Lack of igstkStateMachineMacro() expension - fixed. Expenged to:

void ExportStateMachineDescription( OutputStreamType & ostr, bool skipLoops=false ) const;

• Lack of igstkLoggerMacro() expengion - fixed. Expended to:

protected:
  LoggerType* GetLogger() const;
public:
 void SetLogger(LoggerType* logger);

• Should add a igstkStaticConstMacro(), instead of using itkStaticConstMacro(). refer to
  • igstkCTImageReader.h
  • igstkImageSpatialObject.h
  • igstkImageSpatialObjectRepresentation.h
  • igstkCTImageReader.h
• itkEventMacro?? refer to
  • igstkSerialCommunication.h
• Lack of igstkLogMacro() & igstkLogMacroStatic() expension. how?

#define igstkLogMacro( x, y)  \
{         \
  if (this->GetLogger() ) \
    {  \
    ::itk::OStringStream message; \
    message << y; \
    this->GetLogger()->Write(::itk::Logger::x, message.str()); \
    }  \
}

• Building Doc warinings:

C:/Patrick/IGSTK/IGSTK/Source/igstkTransform.h:53: Warning: namespace igstk already has a brief description. Skipping the documentation found here.
C:/Patrick/IGSTK/Sandbox/Source/igstkNDICommandInterpreter.h:596: Warning: unable to resolve reference to `GetPHSRHandle(int i)' for \ref command

• Problems? when including itk&vtk classes, (#include "xxx.h") xxx.h not linked to the proper external class,

Iteration 6

Please follow the link below:

Iteration 6 Code Reviews