Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 17/12/2012 09:18, address@concealed a écrit :
> Revision
>     8261
> Author
>     sikeda
> Date
>     2012-12-17 09:18:15 +0100 (lun. 17 déc. 2012)
>
>
>       Log Message
>
> [dev] split AUTOLOAD of Site to Site and Robot,
> to prevent Site accessors overriding Robot accessors.
I'm not a big fan of AUTOLOAD ... It defeats static analysis, either by 
dedicated tools such as Test::Pod::Coverage, or just by running grep in 
source tree. It also often has side-effects, such as the need to define 
an empty DESTROY method to avoid it being called when an object is 
garbage-collected, for instance.

However, I understand we have quite a large number of attributes 
(despite I don't get the distinction between 'attributes' and 
'parameters' later in your code), and that it would be painful to 
manually repeat an attribute for each of them. So, I could live with it, 
especially as I have no better alternative to suggest :)

However, I'd really prefer all those read-only accessors being called 
get_foo/bar instead of just foo/bar:
- it would be more consistent with other piece of code (see 
Sympa::VOOT::Provider, for instance)
- a get_foo() has an explicit read-only semantics, whereas a foo() 
method raising an exception if an argument is passed is clearly a trap
- some of those attribute names sound much like a verb than a noun:
$robot->request(), for instance, is quite unfortunate, and suggest an action

> +sub AUTOLOAD {
> +    Log::do_log('debug3', 'Autoloading %s', $AUTOLOAD);
> +    $AUTOLOAD =~ m/^(.*)::(.*)/;
> +    my $pkg  = $1;
> +    my $attr = $2;
>
> -=item listmasters
> +    my $type = {};
> +    ## getters for site/robot attributes.
> +    $type->{'RobotAttribute'} =
> +       scalar(grep { $_ eq $attr } qw(etc home name));

> +    ## getters for site/robot parameters.
> +    $type->{'RobotParameter'} = scalar(
> +       grep { $_ eq $attr }
> +           qw(blacklist list_check_regexp loging_condition loging_for_module
> +           pictures_path request sympa)
> +       ) ||
> +       scalar(grep { !defined $_->{'title'} and $_->{'name'} eq $attr }
> +           @confdef::params);
Implementation remarks...

First, the use of scalar() is useless here, as turning an empty list to 
0 doesn't change much for later evaluation in boolean context a few 
lines thereafter.

Second, this whole block of code would be more readable by first 
computing the list of available attribute names, then using a single 
$type variable to store the lookup result.

# generate list of available class attributes
my @attributes = qw(etc home name);
my @parameters =
     qw(blacklist list_check_regexp loging_condition loging_for_module
        pictures_path request sympa),
     map { $_->{'name'} } grep { !$_->{'title'} } @confdef::params

# which type of method do we need here ?
my $type =
     grep { $_ eq $attr } @attributes ? 'attribute' :
     grep { $_ eq $attr } @parameters ? 'parameter' :
                                        undef;

Last, if this code is supposed to be called multiple times, it would be 
more efficient to index those values once for all instead of enumerating 
them each time.

package Robot;

# index available attributes and parameters names
my %attributes = map { $_ => 1 } qw(etc home name);
my %parameters = map { $_ => 1 } (
     qw(blacklist list_check_regexp loging_condition loging_for_module
        pictures_path request sympa),
     map { $_->{'name'} } grep { !$_->{'title'} } @confdef::params
);

sub AUTOLOAD {
...
if      ($attributes{$attr}) {
...
} elsif ($parameters{$attr}) {
...
} else {
    croak "Can't locate object method \"$attr\" via package \"$pkg\"";
}
}

--
Guillaume Rousse
INRIA, Direction des systèmes d'information
Domaine de Voluceau
Rocquencourt - BP 105
78153 Le Chesnay
Tel: 01 39 63 58 31

Attachment: smime.p7s
Description: Signature cryptographique S/MIME