[Octave-patch-tracker] [patch #8856] add function reducevolume

[Philip Nienhuis]

Follow-up Comment #1, patch #8856 (project octave):

Quite (=very) good for a first contribution! Thank you.

Nevertheless :-)  some points I noted in a ~2 minute scan:

Texinfo header:
---------------
* The first deftypefnx should without the trailing x: deftypefn

* The first sentence of the help is OK, but what is lacking is the rest of the
help text, i.c., information on the *required* and the *optional* input
parameters and what output arguments the function returns. Maybe even usage
examples? Octave users shouldn't have to resort to the Matlab on-line help,
should they :-)
E.g., only by perusing the code I noted somewhere that the function requires
either three or six input arguments (according to Matlab it should be 2 or 5).
That should be in the texinfo help text, to be displayed when doing:

help reducevolume

Code style (-nitpicking):
-------------------------
* We prefer ! negation operators rather than ~
In L.67 I read "if (numel (r) ~= 3)". It should rather be != or maybe "if !
(numel (r) == 3)"

* On L.71 "if any (r(:) < 1 | r(:) != fix (r(:)))": The entire if condition
(any ...... ) should be enclosed in parentheses.

* We start comments with double ## rather than single # or %

* L.87 "x = 1:size(data, 2);" size() is a function so we prefer a space
between size and the left paren: "x = 1:size (data, 2);"

General (important!):
---------------------
* I see no tests. Can you provide tests, also for input validation? For code &
style examples see a.o., strread.m



    _______________________________________________________

Reply to this item at:

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

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