Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 25/02/2014 10:22, IKEDA Soji a écrit :
> Sorry Guillaume, I missed some your comments.
>
> On Mon, 24 Feb 2014 20:12:37 +0100
> Guillaume Rousse <address@concealed> wrote:
>
> <<snip>>
>
> > > +## Database and SQL statement handlers
> > > +my ($sth, @sth_stack);
> > I couldn't find any usage of those variables in the code, excepted for
> > systematically pushing the current global sth variable on the stack
> > before assigning it to a new handle for the current operation, and
> > restoring it therefafter. Basically, you're implementing a context
> > saving mechanism, but there isn't ever context to save between two
> > method/function calls.
> >
> > And even if it was useful, it is still a bad idea to use a package
> > variable for this, as it is shared between all instances: the whole
> > point of using objects is to keep their own state private.
> >
> > Basically, every similar pattern:
> >
> > push @sth_stack, $sth;
> > unless ($sth = SDM::do_prepared_query(...)) {
> >         $sth = pop @sth_stack;
> >         return undef;
> > }
> > // do something with $sth
> > $sth = pop @sth_stack;
> >
> > Can safely be replaced by simpler:
> >
> > my $sth = SDM::do_prepared_query(...);
> > unless ($sth) {
> >            return undef;
> > }
> > // do something with $sth
>
> I guess @sth_stack may had been introduced for iterator functions
> of List, get_first_*(), get_next_*().  However, I suppose that
> effort was not successful.
If you need an iterator, you'd better encapsulate it in its own package, 
with its own private internal state, rather than pollute a global 
namespace. See what I did with Sympa::Log::Database::Iterator in 
sympa-cleanup, for instance.

> > > +=over 4
> > > +
> > > +=item moveto
> > > +
> > > +Change email of user.
> > > +
> > > +=back
> > > +
> > > +=cut
> > > +
> > > +sub moveto {
> > > +    my $self = shift;
> > > +    my $newemail = tools::clean_email(shift || '');
> > > +
> > > +    unless ($newemail) {
> > > +       Log::do_log('err', 'No email');
> > > +       return undef;
> > > +    }
> > > +    if ($self->email eq $newemail) {
> > > +       return 0;
> > > +    }
> > > +
> > > +    push @sth_stack, $sth;
> > > +
> > > +    unless (
> > > +       $sth = do_prepared_query(
> > > +           q{UPDATE user_table
> > > +             SET email_user = ?
> > > +             WHERE email_user = ?},
> > > +           $newemail, $self->email
> > > +       ) and
> > > +       $sth->rows
> > > +       ) {
> > > +       Log::do_log('err', 'Can\'t move user %s to %s', $self, $newemail);
> > > +       $sth = pop @sth_stack;
> > > +       return undef;
> > > +    }
> > > +
> > > +    $sth = pop @sth_stack;
> > > +
> > > +    $self->{'email'} = $newemail;
> > > +
> > > +    return 1;
> > > +}
> > I'd rather rename this method as 'set_id', or 'change_id', for
> > consistency with the previous one. Or at least something more explicit,
> > like 'rename': 'moving', for people, means 'to be physically displaced'.
>
> ID is not the user.
No, it's of its attribute.

> "Move" or "rename" feels better than "set" or "change" for me.
>
> "Rename" may mislead that it will change real name (gecos) attribute
> of object.
>
> How about "$user->move_user($email)"?
That's still 'moving' :) And the user is already the suject of the 
action, given than this is an instance method.

Given the method description ("Change email of user") and implementation 
(change "email" attribute both in database and in memory), why not just 
$user->set_email() ?

> > > +=head2 Miscelaneous
> > > +
> > > +=over 4
> > > +
> > > +=item clean_user ( USER_OR_HASH )
> > > +
> > > +=item clean_users ( ARRAYREF_OF_USERS_OR_HASHES )
> > > +
> > > +I<Function>.
> > > +Warn if the argument is not a Sympa::User object.
> > > +Return Sympa::User object, if any.
> > > +
> > > +I<TENTATIVE>.
> > > +These functions will be used during transition between old and 
> > > object-oriented
> > > +styles.  At last modifications have been done, they shall be removed.
> > A simple "grep 'clean_user'" in 6.2 branche shows than there is abolutly
> > no usage of those function, and than they could be dropped right now.
>
> Robot package also have similar thing.
>
> This is tentative functions to ease transition between non-OO
> and OO.  As I wrote in the POD, these are used during refactoring
> work and shall be removed when the work has finished.
Given than this class was never used before, the need of a transition in 
this specific case is quite questionable. Anyway, it seems to be 
finished now.
--
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