[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:
<http://savannah.gnu.org/patch/?8856>
_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/