[Dune-devel] Updates on psurface convert

[Markus Blatt]

Hi,

On Fri, Jul 26, 2013 at 09:39:10PM -0400, Xiaoxue Gong wrote:
> I have made a clean repository in
> https://gitorious.org/gcos/psurface-convert
> Documentation on the format I used on storing psurface information into vtk
> files and documentation on how to replicate my results is included in
> ./doc/REARDME and ./doc/note/

Thanks, now it looks much cleaner. Still I think there are a few
things that could be changed:

+ When I clone the the repository the directory tree looks like this
  psurface-convert/
  |
   -------- dune-surface-convert
            |
            ---- dune
            |
            ---- src
            ...

  Personally I would have expected the files directly in the root
  directory, i.e. without the additional dune-psurface-convert
  directory in between.
  BTW: I think this is a minor issue, and can easily be resolved (even
  later).

+ With your code being a dune module, I would have exspected it to
   adhere to our guidlines.
   - Use CamelCase for the method and class names. Method names start
     with a lowercase letter, class names with an uppercase letter. 
    (e.g. VTKPWriter::IndexMapBack should be VTKPWriter::indexMapBack
   - The root namespace should probably be Dune (you could use a
     nested PSurface namespace inside it)

+ Your code is indented very inconsequently. This makes it very hard
   to read for others. It is probably due to using tabs. You should
   not use them for indentation! There is the possibility to enforce
   not using tabs when commiting with dunecontrol. Please ask on this
   list how to use this with your repository. Most probably you can
   tell your favorite editor not use tabs for indentation.

+ You should make use of information hiding. Currently all the
   methods of VTKPWriter are declared public. You should limit public
   methods to those that should be exposed to user (e.g. for
   writing/reading the data). I can see why you did, as you seem to
   use all methods and members in writevtk. Maybe this should be a
   member function od VTKPWriter, too?

+ Currently one can only write PSurface<2,float>. Is that intended?
   Shouldn't we be able to write others like PSurface<3,double>, too?

+ IMHO vtuIO.[hc]pp should be usable from other modules
   too. Therefore I would move to an appropriate subdirectory below
   dune/ (take a look at the core modules to see what might be
   appropriate). If you stick with the free method writevtk then
   vtuIO.cpp should be put into a library.

+ Please try to document your code along (or even before) writing it
   using the doxygen syntax. Most developers (me, too) are not
   disciplined enough to document their code after writing it. In the
   rare occasion the we are and that there is time left, we are often
   having a hard time understanding it ourselves (which is a shame and
   waste of time). I would recommend documenting it properly _now_.

+ What is up with the weired AC_ARG_WITH clauses in configure.ac? I do not
   think that they are needed and doubt that they serve their intended
   purpose. 

Cheers,

Markus
-- 
Do you need more support with DUNE or HPC in general? 

Dr. Markus Blatt - HPC-Simulation-Software & Services http://www.dr-blatt.de
Hans-Bunte-Str. 8-10, 69123 Heidelberg, Germany
Tel.: +49 (0) 160 97590858  Fax: +49 (0)322 1108991658