Re: Audio playback and classes

[Mike Miller]

On Wed, Jun 19, 2013 at 12:55:58 -0400, Jordi Gutiérrez Hermoso wrote:
> On 19 June 2013 12:40, Vytautas Jancauskas <address@hidden> wrote:
>> What is a good naming convention for these low level functions that
>> will get wrapped by octave class methods? I see @ftp adds two
>> underscores in front.
>
> We use two underscores both front and back for functions that are
> supposed to be internal and not called by the user.

Hi Vytautas, it looks like you're making good progress along this path
so far. It doesn't compile for me at the moment, perhaps you forgot to
commit __player_set_fs__.cc along with your most recent change? Can you
add that file when you get a chance?

I was able to work around that and test your changes to the class. I
don't know if you saw this, but I now get a runtime type error when
constructing an audioplayer. It's important that the strings passed to
DEFINE_OV_TYPEID_FUNCTIONS_AND_DATA are the same as the class name you
want to be used in the interpreter, "audioplayer" in our case.

I believe you are also missing a call to the register_type() function
for the class. You'll want to call that when the first audioplayer
object is constructed.

Do you think that __audioplayer and the __player_* functions should have
a more consistent naming convention between them? If listing them all
with dozens of other functions, it would help to see that they are
related to each other.

For the most part it looks like you are following the Octave coding
style correctly. However, some points to consider:

 - Insert a space before the opening parenthesis of a function call or
   definition, to differentiate from the matrix indexing operator.

 - Check for argument errors early instead of leaving it to an else-case
   that may get lost further down in the function.

For example in audioplayer.m, you currently have the following:

  if (nargin == 2)
    player.player = __audioplayer(varargin{1}, varargin{2});
  elseif (nargin == 3)
    player.player = __audioplayer(varargin{1}, varargin{2}, varargin{3});
  elseif (nargin == 4)
    player.player = __audioplayer(varargin{1}, varargin{2},
varargin{3}, varargin{4});
  else
    error('audioplayer: wrong number of arguments passed to the constructor')
  endif

I would reduce this to the more concise (and consistent with the rest of
Octave):

  if (nargin < 2 || nargin > 4)
    print_usage ();
  endif

  player.player = __audioplayer (varargin{:});

This is looking really good so far. Keep up the good work!

-- 
mike