[Insight-developers] RE: FEM toolkit (pre-alpha)

[Aljaz Noe]

First of all, I must apologize for not attending the TCON. I was told it
starts at 2pm. Sorry...

Next I want to reply to your comments about the code. In short...


> - .cpp files should be named .cxx

No problem.


> - the documentation needs work.
>   + not enough

We'll provide the amount of comments in the code to match existing itk
documentation.


>   + it is not consistent with the rest of the system

We'll change the style to comply with the Doxygen and the rest of the itk.


>   + needs copyright headers on every file

No problem...


> - the coding style is inconsistent. {} are placed after for,
> while, if, and are indented two spaces, along with the code
>    + for ()
>        {
>        ....code here....
>        }

Will be corrected.


>    + data members should be private, with Set/Get methods

We can do that. Also see comment below.


>    + missing manual instantiation #ifdefs

Will be corrected.


>    + constructors and destructors should be protected
>    + operator= and copy constructor should be private and not implemented

See comment below.


>    + class names are wrong: itkFEMNode2dIsotropic does not need
> the leading itk (since its in the itk namespace); 2d should be 2D;

Will be changed.


>    + Method names are inconsistent: SkipWS() should be SkipWhiteSpace().

Will be changed.


>    + Not using New() and Delete() that I could see.
>    + Not using smart pointers that I could see.
>    + Not inheriting from Object or LightObject, or at least providing a
good reason why.

Regarding the use of itk's smartpointers (lightobjects, object
factories... ) in all of the FEM classes, I'll have to discuss this with the
rest of the people involved. There seem to be several different approaches
to changing the code. Each one having its pros and cons. We will discuss
this in detail next week and will post the options we have together with
pros and cons as soon as we properly formulate them.


> I recommend that you read the style guide. I recommend that you
> look at the rest of the system and make your stuff consistent
> with it.

We'll fix the style.


> I don't see how this stuff can be ready for the beta.

Maybe not. We'll do our best...


> You may wish to create a fem namespace. I'd also suggest creating
> another Doxygen group/module and adding some documentation.

Inside the itk namespace (::itk::fem::Element) or outside (::fem::Element)?


> Please clean this up and we'll look at the architecture.

We are already cleaning it... ;-)


Aljaz