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

[Will Schroeder]

Hi Aljaz-

Here's some feedback on the FEM classes that you sent. It's in no particular order. There are some general comments at the end.

- .cpp files should be named .cxx
- the documentation needs work.
  + not enough
  + it is not consistent with the rest of the system
  + needs copyright headers on every file
- the coding style is inconsistent. {} are placed after for, while, if, and are indented two spaces, along with the code
   + for ()
       {
       ....code here....
       }
   + data members should be private, with Set/Get methods
   + missing manual instantiation #ifdefs
   + constructors and destructors should be protected
   + operator= and copy constructor should be private and not implemented
   + class names are wrong: itkFEMNode2dIsotropic does not need the leading itk (since its in the itk namespace); 2d should be 2D;
   + Method names are inconsistent: SkipWS() should be SkipWhiteSpace().
   + 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.

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. I don't see how this stuff can be ready for the beta.

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

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

The Grinch