Re: new function: textscan.m

[Ben Abbott]

On Oct 23, 2010, at 8:53 AM, John W. Eaton wrote:

> On 23-Oct-2010, Ben Abbott wrote:
> 
> | I've made an attempt to implement the missing function textscan.m
> | 
> | If there are no suggestions for improvement, I'll commit.
> 
> +  if (nargin > 2 && isnumeric (varargin{1}))
> +    N = varargin{1};
> 
> I think it would help to quickly understand what N is if you used
> nlines or similar instead of N.  Also, we generally try to avoid
> uppercase variable names in Octave.
> 
> +  if ((! strcmp (class (fid), "double") || fid < 0) && ! ischar (fid))
> +    error ("textscan: first input argument must be a valid file id, or 
> string.");
> +  endif
> +
> +  if (! ischar (formatstr) && ! isempty (formatstr))
> +    error ("textscan: second input must be a format specification.");
> +  endif
> 
> Maybe I'm just slow, but I have a harder time understanding negative
> conditions like the ones above.  Instead of checking the conditions
> that lead to errors, I find it simpler to write and easier to
> understand code later if I test the conditions for success instead.
> For example, instead of the above, I would write something like
> 
>  if (isa (fid, "double") && fid > 0 || ischar (fid))
>    if (ischar (formatstr) || isempty (formatstr))
>      ## ... code to do the real work here ...
>    else
>      error ("textscan: second input must be a format specification");
>    endif
>  endif
>  else
>    error ("textscan: expecting first argument to be a file id or character 
> string");
>  endif
> 
> Is that condition on formatstr correct?  Is it OK for it to be empty
> if it is not a character string?
> 
> Note also that isa is probably better than class+strcmp.  But what
> happens if fid is a matrix?  Should we check for that?  Should we
> maybe have a is_valid_file_id function?  Maybe that would also be
> useful in other places too.
> 
> jwe

I've included your suggestions. Regarding a is_valid_file_id() function, I like 
the idea. Can this be done as an m-file?

My 2nd attempt is attached.

Ben

changeset.patch
Description: Binary data