Developers of Sympa

[IKEDA Soji <address@concealed>]

Guillaume,

I entirely agree to you.  Essentially List::*_global_user() would
have to be replaced with corresponding OO methods of Sympa::User.

Incomplete transition from non-OO to OO is intentional.  It was
made so that 6.2 and trunk will be in nearly sync as much as possible.
Complete transition (including appropriate naming of variables) may
be made at 7.0, I suppose.

Regards,

--- Soji

On Mon, 24 Feb 2014 20:12:37 +0100
Guillaume Rousse <address@concealed> wrote:

> Whereas turning a recurrent data structure into a full-fledged class is 
> a good idea, the current implementation suffers from a few design issue, 
> including useless complexity first, and an ugly mix of OO and non-OO 
> style into the same logical unit (package).
> 
> More comments below.
> 
> Le 23/02/2014 12:33, address@concealed a écrit :
> > --- branches/sympa-6.2-branch/soap/sympasoap.pm     2014-02-23 11:20:57 
> > UTC (rev 10268)
> > +++ branches/sympa-6.2-branch/soap/sympasoap.pm     2014-02-23 11:33:58 
> > UTC (rev 10269)
> [..]
> > @@ -765,13 +765,13 @@
> >       }else {
> >     my $u;
> >     my $defaults = $list->get_default_user_options();
> > -   my $u2 = &List::get_global_user($email);
> > +   my $u2       = Sympa::User->new($email);
> cosmetic issue: 'my $new_user' instead of 'my $u2' would be more 
> readable, and more consistent with other usages.
> 
> [..]
> > @@ -1350,10 +1350,11 @@
> >         }
> >
> >         if ($List::use_db) {
> > -     my $u = &List::get_global_user($sender);
> > -   
> > -     &List::update_global_user($sender, {'lang' => $u->{'lang'} || 
> > $list->{'admin'}{'lang'}
> > -                                     });
> > +            my $u = Sympa::User->new($sender);
> > +            unless ($u->lang) {
> > +                $u->lang($list->{'admin'}{'lang'});
> > +                $u->save();
> > +            }
> >         }
> cosmetic issue: 'my $user' insteas of 'my $u'
> 
> >         ## Now send the welcome file to the user
> >
> >
> >         Modified: branches/sympa-6.2-branch/src/lib/Commands.pm (10268
> >         => 10269)
> >
> >
> > --- branches/sympa-6.2-branch/src/lib/Commands.pm   2014-02-23 11:20:57 
> > UTC (rev 10268)
> > +++ branches/sympa-6.2-branch/src/lib/Commands.pm   2014-02-23 11:33:58 
> > UTC (rev 10269)
> [..]
> > @@ -1294,11 +1294,10 @@
> >     }
> >     
> >     if ($List::use_db) {
> > -       my $u = &List::get_global_user($email);
> > -   
> > -       &List::update_global_user($email, {'lang' => $u->{'lang'} || 
> > $list->{'admin'}{'lang'},
> > -                                      'password' => $u->{'password'} || 
> > &tools::tmp_passwd($email)
> > -                                       });
> > +       my $u = Sympa::User->new($email);
> > +       $u->lang($list->{'admin'}{'lang'}) unless $u->lang;
> > +       $u->password(tools::tmp_passwd($email)) unless $u->password;
> > +       $u->save;
> >     }
> >     
> >     ## Now send the welcome file to the user if it exists and 
> > notification is supposed to be sent.
> > @@ -1683,7 +1682,7 @@
> >         &Log::do_log('debug2','Sending REMIND * to %d users', $count);
> >
> >         foreach my $email (keys %global_subscription) {
> > -           my $user = &List::get_global_user($email);
> > +           my $user = Sympa::User::get_global_user($email);
> Why two different ways to achieve the same result ?
> 
> The previous usage of List::get_global_user() was replaced by a call to 
> Sympa::User->new(), this one is replaced by a call to 
> Sympa::User::get_global_user(). That's confusing, at best.
> 
> Additionaly, the mix of OO style (new() is a method class) and 
> procedural style (get_global_user() is a function) seems a bad idea: a 
> given package should only have OO-style public interface, or only 
> procedural interface, not both.
> 
> [..]
> > --- branches/sympa-6.2-branch/src/lib/Sympa/User.pm                       
> >   (rev 0)
> > +++ branches/sympa-6.2-branch/src/lib/Sympa/User.pm 2014-02-23 11:33:58 
> > UTC (rev 10269)
> > @@ -0,0 +1,656 @@
> > +# Sympa - SYsteme de Multi-Postage Automatique
> > +#
> > +# Copyright (c) 1997, 1998, 1999 Institut Pasteur & Christophe Wolfhugel
> > +# Copyright (c) 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005,
> > +# 2006, 2007, 2008, 2009, 2010, 2011 Comite Reseau des Universites
> > +# Copyright (c) 2011, 2012, 2013, 2014 GIP RENATER
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +package Sympa::User;
> > +
> > +use strict;
> > +use warnings;
> > +use Carp qw(carp croak);
> > +
> > +#use Site; # this module is used in Site
> > +
> > +## 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
> 
> [..]
> > +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.
> 
> > +=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'.
> 
> [..]
> > +############################################################################
> > +## Old-style functions
> > +############################################################################
> > +
> > +=head2 OLD STYLE FUNCTIONS
> > +
> > +=over 4
> > +
> > +=item add_global_user
> > +
> > +=item delete_global_user
> > +
> > +=item is_global_user
> > +
> > +=item get_global_user
> > +
> > +=item get_all_global_user
> > +
> > +=item update_global_user
> > +
> > +=back
> > +
> > +=cut
> Those should either be kept as private functions, not usable from 
> outside the package, either turned as class methods instead.
> 
> Additionaly, those names refer to the concept of 'global user', but is 
> there anything like 'non-global user' in Sympa that would make this 
> distinction useful ?
> 
>  From the class description, which refers to 'identified users', I think 
> a better terminology here would be 'registered users', or 'known users'.
> 
> [..]
> > +=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.
> 
> -- 
> Guillaume Rousse
> INRIA, Direction des systèmes d'information
> Domaine de Voluceau
> Rocquencourt - BP 105
> 78153 Le Chesnay
> Tel: 01 39 63 58 31
> 
> 


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