Developers of Sympa

[Guillaume Rousse <address@concealed>]

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

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