Coding Guidelines
A recent examination of all perl code in the perfSONAR code base has turned up many different programming styles in use. This is a common problem in OSS projects involving developers of many different backgrounds and skill sets. We are aiming to establish several guidelines to make the code uniform, reliable and functional (and to make code review easier).
This list is not complete but a reflection of examples from already existing code. Also, this list is open for additions but please be reasonable and base your suggestions on really "best for large project" practices.
Read perl best practices book by Damian Conway. 90% of the items related to code layout and naming conventions are very accurate (cant say the same about other policies, use them on your own risk). Perl coding has changed a lot in the past 10 years, please dont apply examples from cookbook if the are older than ~3-5 years.
Despite Conway's book, please use POD in front of each function/method definition and not in the bottom or as simple comments in the body of the function like in this example:
sub handleRequest($$$) {
my ($handler, $request, $endpoint_conf) = @_;
# This function is a wrapper around the handler's handleRequest
# function. The purpose of the function is to handle the case where a
# crash occurs while handling the request.
It will make other person happier if he will be able to read semantic of the function, accepted parameters and returns, all in one place in front of the function definition ( private methods might be documented ad hoc). A good example:
=head2 functionName( args )
This function is a ....
accepts parameters:
returns:
=cut
sub functionName {
}
To keep the code consistent, all perl modules should use the following POD template:
package perfSONAR_PS::NAME;
use fields 'LOGGER';
use strict;
use warnings;
use version;
our $VERSION = qv("XXX");
=head1 NAME
perfSONAR_PS::NAME
=head1 DESCRIPTION
description
=head1 API
=cut
use Log::Log4perl qw( get_logger );
use Params::Validate qw( :all );
use perfSONAR_PS::ParameterValidation;
=head2 new( $package, { } )
description
=cut
sub new {
# function goo
}
=head2 function( $self, { param } )
description
=cut
sub function {
# function goo
}
# other functions
1;
__END__
=head1 SYNOPSIS
#!/usr/bin/perl -w
use strict;
use warnings;
use perfSONAR_PS::NAME;
# example use...
=head1 SEE ALSO
L<Log::Log4perl>, L<Params::Validate>, L<perfSONAR_PS::ParameterValidation>
To join the 'perfSONAR Users' mailing list, please visit:
https://mail.internet2.edu/wws/info/perfsonar-user
The perfSONAR git repository is located at:
http://github.com/perfsonar
Questions and comments can be directed to the author, or the mailing list.
Bugs, feature requests, and improvements can be directed here:
http://github.com/perfsonar/project/issues
=head1 VERSION
VERSION info or version control ID string
=head1 AUTHOR
John Q Author, jqa@asdf.org
=head1 LICENSE
YOUR LICENCE
=head1 COPYRIGHT
Copyright (c) 2009, YOUR ORG
All rights reserved.
=cut
Use indention of 4 spaces everywhere.
Use one space to seperate the operation and bounding characters of all operations. For example loops:
foreach my $x ( @array ) {
# loop stuff
}
or
foreach my $x ( @{ $array } ) {
# loop stuff
}
Ifs:
if ( exists $content->{ "first" } and $content->{ $first } ) {
}
else {
}
Assignments:
my $length = $# { $hash->{ "array" }->[0] };
or
my $var = 4 + ( $array * eval ( $hash->{ "stuff" } ) );
And finally print statements:
print "One $motion over the " . $bird . "'s nest.\n";
Brace and parenthesize are up to the module author (it is more about personal taste) but please be consistent. This is fine:
if ( defined $content and $content ) {
# something
}
else {
# something else
}
Or this:
if ( defined $content and $content )
{
# something
}
else
{
# something else
}
Or this:
if ( defined $content and $content ) {
# something
} else {
# something else
}
Do not do this for the single line case:
if ( defined $content and $content )
# something
else
# something else
This is acceptable:
print "This" if $that;
Dont use this:
sub handleRequest($$$)
Or this:
sub new {
my ($package, $ns, $port, $listenEndPoint, $contactHost, $contactPort, $contactEndPoint) = @_;
Or even this:
sub createSubjectL3
{
my $doc = shift;
my $ns = shift;
my $baseNS = shift;
my $protocol = shift;
my $sourceAddress = shift;
my $destinationAddress = shift;
This is very error prone and makes debugging hell. If you have more than 1 parameter (except for object self) then ALWAYS use hashref with named parameters like that:
use Params::Validate qw( :all );
use perfSONAR_PS::ParameterValidation;
sub function {
my ( $self, @args ) = @_;
my $parameters = validateParams(
@args,
{
arg1 => { type => Params::Validate::ARRAYREF | Params::Validate::UNDEF },
arg2 => { type => Params::Validate::HASHREF | Params::Validate::UNDEF, optional => 1 },
arg3 => 0
}
);
# code...
}
This implies that besides self, arg1 is rquired and should be a ARRAYREF. Arg2 and Arg3 are optional, with Arg2 being a HASHREF wile the type of Arg3 is undefined.
return arrayref if you need to return multiple parameters or hashref if they should be keyd.
Check when dereferencing hashrefs or arrayrefs, this line:
foreach my $port (keys %{ $conf{"port"} }) {
will break at one time when $conf{port} is undef.
Don't put:
#!/usr/bin/perl -w -I ../lib
in the first line of your module/class, because it doesn't make sense and even if you tested your module by doing this then it does not mean it will work on other host under other environment with other perl executable.
Please avoid dealing with multiple configuration parameters in the main script, move processing somewhere and use standard modules.
use fields;
for your class public ( and private ) fields.
This is an example of bad design:
sub registerLS($) {
my ($args) = @_;
%child_pids = ();
Try to create pass state object instead or create queue object.
Multi-line print statements are expensive, particularly when using quotes, e.g.:
print "\"first\" thing\n";
print "\"second\" thing\n";
Is less efficient than:
print qq{
"first" thing
"second" thing
};
Note that:
print q{ ... };
Works for single quote situations.
We must establish single rule which must be supported throughout the API about what is an error return of some function and what is the correct return. I've seen 0 as error, 0 as correct, -1 as error, "" as error, undef as error, error message as an error. So lets consider set of rules (please add or change any):
- if function must return something then undef is an error ( it should be explicitly expressed in the function name by using "get" as prefix )
- if function may or may not return then -1 is an error ( except when function operate over negative numbers, but this case is extremely rare)
- if function should not return anything then -1 is an error condition
- Always use return , don rely on the last expression returned value!
Correctness might be achieved by negating error condition
As an alternative solution I would like to suggest adding something called ErrorCode object into the API, and returning instance of it in case of error. Then error condition can be easily checked by saying :
my $return = mySub($myparam);
if($return && blessed $return && ref($return) eq 'ErrorCode') {
do somehting about it...
}
The ErrorCode might be a hash based object with code and value keys. This will lead us into establishing single place for quantified error codes/messages. This ErrorCode can be subclass of Error::Simple module from CPAN.
Constructor of the class must support inheritance, period. Its even better if it could be a copy constructor as well ( Clone::Fast is good).
use version;
our $VERSION = qv("2.0");
Is way better alternative to just saying:
our $VERSION = "2.0";
use base;
Instead of:
@ISA = something;
This will try to load the superclass at compile time. Try to avoid multiple inheritance: this is the hellish way which leads into creating namespace conflicts.
Write tests at the same time as your modules are written. Automate tests generation if possible.
Branches should be created for development efforts that could take multiple days, may be disruptive, or if the submission needs be reviewed by others. Small fixes can be done directly in trunk, but should be well tested before being checked in. Q/A should take place when moving code from trunk to a release branch.