Developers of Sympa

[David Verdin <address@concealed>]

Le 26/02/14 10:31, Guillaume Rousse a écrit :

address@concealed">Le 25/02/2014 11:48, David Verdin a écrit :

However, I think it is important to test the presence / absence of
expected parameters just after the "my (%params) = @_;" and translate
them into local variables. We could even undef the %params hash just
after converting its values to local variables.
That way, if we had a new key/value to the params hash and try to use it
directly in the code a $hash{$key}, we woul find the b_ug at compile time.

Multiple issues here.

Testing the presence (and eventually the type) of expected parameters matters, indeed, but I'd restrict it to public functions/methods only. For private functions/methods, that are supposed to be called from the same package only, this is mostly useless, and should be assumed to be done by the caller.

# this function can be called by any part of sympa
sub function {
    my (%params) = @_;

    # check params first
    if (!$params{a}) {
        # handle error
    }

    # do something
}

# this function can be called only by current package
sub _function {
    my (%params) = @_;

    # assume parameters have been checked by caller

    # do something
}

Second, assuming than copying parameters into local variables always brings additional benefits from compilation-time checks is discussable, especially for short functions, ie:

sub function {
    my (%params) = @_;

    return $params{a} . '@' . $params{b};
}

sub function {
    my (%params) = @_;

    my $a = $params{a};
    my $b = $params{b};

    return $a . '@' . $b;
}

The potential typo issue in the actual code of the function in the second form is actually just displaced in the local variable affectation: no safety gain, with two additional variable copies.

Third, the ability to reuse initial parameters list directly is very useful in some circunstances, such as wrapping functions with other functions, ie:

sub generic_function {
    my (%params) = @_;
    ...
    return $something;
}

sub less_generic_function {
    return generic_function(@_, foo => 'value');
}

That's exactly the case of current wwslog() function, for instance, that just wraps do_log() function. I wouldn't call it a bug, and even less enforce a mandatory removal of initial parameters list.

Last, I think it is not reasonable to try to enforce a unique fit-them-all rule. Especially with regard to the current code base, the lack of workforce to enforce it, and the difficulties we have to find consensus, even on the smallest cosmetic issues.

The following example should adress the various concerns, and constitute a suitable *prefered* template for public functions/methods (ie: not the absolute rule):

sub function {
    my (%params) = @_;

    if (!$params{c}) {
         # log error and return
    }

    $a = defined $params{a} ? $params{a} : 'default for A';
    $b = defined $params{b} ? $params{b} : 'default for B';
    $c = $params{c);

    ...
}

Is that acceptable for everyone ?

You make a valid poiunt here. I tend to enforce a uniform style whereas a uniform good sense is often enough.
I like this template and woul certainly embrace it - safe for exceptions.

Cheers,

David

--
A bug in Sympa? Quick! To the bug tracker!

David Verdin Études et projets applicatifs
Tél : +33 2 23 23 69 71 Fax : +33 2 23 23 71 21   www.renater.fr	RENATER 263 Avenue du Gal Leclerc 35042 Rennes Cedex

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