[Octave-patch-tracker] [patch #8872] add isocaps

[Lachlan Andrew]

Follow-up Comment #5, patch #8872 (project octave):

Thanks, Markus.

I forgot to mention that we also need to change all docstrings to replace
{Function file} by {}.

Other comments are:
1. Are you sure which_caps can only be a/above or b/before?  If not, it may be
better to make "b", "before" a separate case and have "otherwise" throw an
error.

2. I think in-line comments should start with a single "#".

3. It is expensive to building faces and vertices one element at a time like
in the 'if (strcmpi (which_plane, "all"))' since it requires a copy each time
the matrix grows.  It would be faster to allocate f1, f2, ..., f5, and then
form [faces; f1; f2; f3; f4; f5].  Similarly, it is faster to build
vertices_grid from row 3 to row 1, or to preallocate it.

4. Several of the lines are too long.

5. It would be good it the docstring didn't assume that the reader knows what
end-caps are.

6. I'm not sure, but I think you can replace


@table @asis
@item @code{above}


by


@table @code
@item above


If so, the latter probably expresses your intent better.

7. __get_isocaps_patches__ seems quite repetitive, which makes maintenance
hard.  Is it possible to use the switch statement to choose a small number of
parameters (like which dimension to use), and then have a single isosurface
command?  That may end up being less clear (which is also a maintenance
issue).

Hmm... You have submitted lots of patches (thank you!) and it will take a long
time to review them all.  Would you like to prioritise your patches, with the
ones you think should be reviewed first?

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?8872>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/