[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philip Nienhuis
Subject: [Octave-patch-tracker] [patch #8856] add function reducevolume
Date: Wed, 13 Jan 2016 23:09:58 +0000
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0 SeaMonkey/2.38

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:


  Message sent via/by Savannah

reply via email to

[Prev in Thread] Current Thread [Next in Thread]