criticisms, comments on newest module please
Peter Scott
Peter at PSDT.com
Wed Jan 22 14:03:04 CST 2003
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/
More information about the Victoria-pm
mailing list