criticisms, comments on newest module please

[Peter Scott]

At 11:50 AM 1/22/03 -0800, Darren Duncan wrote:
>On Wed, 22 Jan 2003, nkuipers wrote:
> > First of all, I know everyone is busy, and that taking the time to 
> critique a
> > newby module is a bit of a hassle, so it's not a problem if I get no
> > responses.  But since one of my goals this year is to improve my module
> > writing skills, I figure that it is valuable to me to have peers review my
> > work.  I'm not after criticism of the "what" (which will undoubtedly be
> > fleshed out as new functionality occurs to me) so much as the "how" 
> and "why",
> > the style, the housekeeping within the module, etc.  Any comments or
> > suggestions are greatly appreciated.  I'll roll out documentation when the
> > technical aspects are satisfactory, of course.
>
>At first glance the module looks quite satisfactory, although I don't know
>much about what it is supposed to do.  I can look more closely at it
>during the following few days, but at the moment I can offer these
>suggestions:
>
>- Put the "use strict" pragma above *all* lines in the file, except the
>"package" line.

Agreed.  Ditto "use warnings", unless the main program uses -w.  I've 
taken to "use warnings" everywhere so I can selectively disable them 
where necessary.

>- Instead of saying "[action] if/unless [condition]", say either
>"if/unless ( [condition] ) { [action] }" or "[condition] and/or
>[action]".  I find code much easier to understand if the cause comes
>before the effect, even if Perl supports the other approach as well.

Just as another point of view :-) perlstyle makes the IMHO valid point that

          On the other hand

              print "Starting analysis\n" if $verbose;

          is better than

              $verbose && print "Starting analysis\n";

          because the main point isn't whether the user typed -v
          or not.

I follow that style.  When I want the test to precede the action I use 
'and', viz:

         $nuts > 0 and hibernate();

I have a convention of using or/and for things that I think of as 
changing the flow of control and ||/|| where I want to use the result 
of the expression.

>- Do not export any symbols into the caller's namespace.  Since you are
>already using objects, just let the caller say "Classname->func()" or
>"$object->method()".  This makes calling code easier to understand, and
>your functions or methods won't conflict with any that the caller declares
>themselves.  Besides, in order for the caller to use the exported function
>names in the way that exporting provides, they would have to say
>"function( $obj, @args )" rather than "$obj->function( @args )".

Totally agree.  Especially since the convention is that methods 
beginning with _ are private and you've exported some of those too.

Your AUTOLOAD is redundant.  You might consider using it for the 
accessors, though, viz :

sub AUTOLOAD {
         (my $meth = our $AUTOLOAD) =~ s/.*:://;  # Untested
         {id => 1, desc => 1, seq => 1}->{$meth} and return $_[0]->{$meth};
         carp "Called undefined subroutine \"$meth\"";
}

Then you can get rid of the id, desc, seq subs...

--
Peter Scott
Pacific Systems Design Technologies
http://www.perldebugged.com/