Developers of Sympa

[IKEDA Soji <address@concealed>]

Hi,

At first, I'll write about my initial prespective on refactoring
about Site-Robot-List(-Family) classes.

o Site, Robot and List objects are unique actors on Sympa.  Each
  of them can:

  - have e-mail addresses (and maybe URLs) distinguishing them
    from others,

  - send system messages using addresses above,

  - find templates, scenarios etc. from hierarchy in chage, and

  - have their particular configuration parameter sets.

o Core features described above may be aggregated into single
  base class and derived by each subclasses.  (Current name
  "Site_r" is tentative).

o Family have just a part of core features above.
  Currently It is in the subclass chain.

  But I suppose it may be rather represented as the plugin
  overriding function of Robot to force constraint on List
  configurations (In other words, Robot would behave as the Family
  with fully-permissive constraint).
  # It will make create_list() and create_list_old() be unified.

o Each modules should be self-reliance: That is, once Site module
  has been loaded, core features provided for Site should be
  available.

On Mon, 29 Jul 2013 15:05:30 +0200
Guillaume Rousse <address@concealed> wrote:

> Le 25/07/2013 14:46, David Verdin a écrit :
<<snip>>
> >>> Second, the spreading of attribute definition breaks object-modeling
> >>> assumption than classes should be self-contained: the list of attributes
> >>> for a robot object should be defined in Sympa::Robot object, not
> >>> somewhere else (its specific attributes, of course, inherited ones being
> >>> defined in its parent classes)
> > Well at first, with the new code written by Soji, I was confused. I
> > didn't know where the subs used were defined. But then I found really
> > easy to have only a small sub to call to access a config parameter.
> I have no objection having a method defined only once in a parent class. 
> That's one of the basic ideas of inheriting from another class.
> 
> But that's only true if this definition is actually unique, and is not a 
> giant switch enumerating all possibles subclasses, as most methods in 
> Site and Site_r actually do. For instance, here is the dynamically 
> created accessor in Sympa::Site:
> 
> # common case
> 
> if (ref $self and ref $self eq 'Sympa::Robot') {
>    # Sympa::Robot specific case
> } elsif ($self eq 'Sympa::Site') {
>    # Sympa::Site specific case
> } else {
>    croak "fatal error";
> }
> 
> That's exactly the same kind of pattern that was previously used in 
> Sympa::Database::connect() method, to handle database-specific pre or 
> post-connection handling. And this is plainly wrong, because it 
> introduces a strong coupling between the parent class and the child 
> class. The parent should only define its own behaviour, and the child 
> should override the method to enforce its own.
> 
> Here is a better alternative:
> 
> package Sympa::ConfigurableObject;
> 
> =name1 An abstract class for all classes using configuration parameters
> 
> sub get_parameter {
>      my ($self, $parameter) = @_;
> 
>      # check the parameter name
>      my $class = ref $self;
>      if (! $class::params{$parameter}) {
>          # handle 'no such parameter' case
>      }
> 
>      # check initialisation status
>      if (! $self->{parameters}) {
>          # handle 'uninitialized' case
>      }
> 
>      return $self->{parameters}->{$parameter};
> }
> 
> package Sympa::List;
> 
> use base qw(Sympa::ConfigurableObject);
> 
> # flat (but ordered) list of parameters
> our @parameters = (
>      {
>          name       => subject,
>          group      => 'description',
>       gettext_id => "Subject of the list",
>       format     => '.+',
>       occurrence => '1',
>       length     => 50
>      },
>      ...
> );
> 
> # the same list, indexed by parameter name
> our %parameters = map { $_->{name} => $_ } @parameters;
> 
> package Sympa::Site;
> 
> use base qw(Sympa::ConfigurableObject);
> 
> etc...
> 
> 
> This way:
> - each class in self-contained (parameters for Sympa::Site are defined 
> in Sympa::Site namespace, parameters for Sympa::List are defined in 
> Sympa::List namespace, etc...), but still accessible from outside if needed.
> - the base class has a human-understandable name
> - get_parameter() if needed is defined only once, but could eventually 
> get overriden if needed

I have no objection to your proposal.  Currently processing of Site
and Robot parameter sets are overwrapped, because Site parameter set
contains both site-wide parameters and defaults for Robot parameters.
They can be separated apropriately.

Other features such as send_file() may be devided into generic and
class-specific parts.

Additionally, I think Sympa::Configuration would be purified to
become one of drivers for various configuration backend.

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