Developers of Sympa

[IKEDA Soji <address@concealed>]

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.


> > +sub get_id {
> > +    ## DO NOT use accessors since $self may not have been fully 
> > initialized.
> > +    shift->{'email'} || '';
> > +}
> An explicit return statement would be easier to read. Especially for 
> people not reading Perl natively.

I have no objection.


> > +=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.

"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)"?


> > +=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.


Regards,


--- Soji


-- 
株式会社 コンバージョン  セキュリティ&OSSソリューション部   池田荘児
〒231-0004　神奈川県横浜市中区元浜町3-21-2 ヘリオス関内ビル7F
e-mail address@concealed  TEL 045-640-3550
http://www.conversion.co.jp/