Developers of Sympa

[IKEDA Soji <address@concealed>]

On Tue, 18 Mar 2014 14:34:50 +0100
Guillaume Rousse <address@concealed> wrote:

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

I agree (I didn't write these functions).

In reality, these functions are used to get entire result of given
query.  I wonder if iterator interface is necessary.


> >>> +=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() ?

This function does not only "set" e-mail address of the user.
The address is guarantied its uniqueness: If the same address
existed in user_table, this function fails.

So it may be expressed that (1) "the user moves to specified e-mail
address".  Or, if it is described as a change of an attribute, it
may be expressed that (2) "the user changes its e-mail" or "chage
the e-mail of user".  It may also be expressed that (3) "move user
to specified e-mail address", as an example in my previous post.

(1) $user->moveto(EMAIL);
(2) $user->change_email(EMAIL);
(3) $user->move_user(EMAIL);

(2) and (3) are a product of compromise on the convention proposed
by you ("verb_noun" style).  I prefer to (1), and I can accept (2)
or (3).


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

Previously (and currently, on 6.2beta), Robot was represented by
the string containing its "domain".
Similarly, User was represented by hashref.

These transitions were done very recently and have not be proven to
be finished.  So I wish these tentative functions left for a bit
more moment.


Regards,

--- Soji

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