Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 29/05/2013 05:02, IKEDA Soji a écrit :
> Hi,
>
> On Fri, 24 May 2013 15:57:35 +0200
> Marc Chantreux <address@concealed> wrote:
>
> > hello,
> >
> > On Fri, May 24, 2013 at 02:12:04PM +0200, Guillaume Rousse wrote:
> > > > so we can use roles to check that required are implemented
> > > I prefer this class hierarchy, which clearly separates concerns.
> >
> > i'm not sure about this "this" in this assert :)
> >
> > > However, is there any interest to have some abstraction over spool
> > > content (message, key, whatever) ? After all, the calling code
> > > usually know which is expected content over each spool...
> >
> > > And have each of the two final classes implement distinct methods
> > > for storing distinct kind of content:
> > > Sympa::Spool::File::add_message()
> > > Sympa::Spool::File::add_key()
> > > Sympa::Spool::File::add_something_else()
> >
> > It scares me right now as we'll quickly add some
> >
> > * if i have a spool with only 2 methods (say add and delete), i'm happy
> >    to see only 2 functions in the code and in the doc.
> > * what if you have 2 spool means have incompatible methods: you'll need
> >    extra code ?
> >
> > IHMO: Spool provides a basic interface that any spool mean can extend.
> > * Spool::Message ISA Spool
> > * Spool has Spool::Backend
> > * Spool::* contains every business method
> >
> > > This doesn't prevent to have mixed-content spool by mistake, but
> > > code readability also helps to prevent coding errors :)
> >
> > I don't thing splitting things this way would add a lot of complexity
> > but you might want to share some set of methods over Spool types: that's
> > why the roles are.
> >
> > Basically: spools can be queues, trees, ordered or not, thread-safe or
> > not, ... maybe it's more that just a backend.
> >
> > i'm confused for now. let the week-end help :)
>
> It comes to me that internal data (message content, key and
> something else) can be packed into single object.  It will make
> things simple: Subclassing of Spool class may not be neccessary.
>
> ------------ 8< ---------------- 8< ---------------- 8< ------------
> package Spool;
>
> sub new {
>      my $pkg = shift;
>      my $spoolname = shift;
I tried to enforce the use of direct assignation from @_ everywhere:
my ($class, $name) = @_;

My goal was to keep @_ intact, and be able to pass it directory to some 
kind of log_function_call(), instead of painfully enumerating local 
variables each time.

>      if ($spoolname eq 'incoming') {
>         return bless {
>             'spoolname'  => 'msg',
>             'generator'  => 'Message',
>             'storagemgr' => 'Backend::File',
>         } => $pkg;
>      } elsif ($spoolname eq 'pending') {
>         return bless {
>             'spoolname'  => 'mod',
>             'generator'  => 'Message::Keyed', # message with key
>             'storagemgr' => 'Backend::SQL',
>         } => $pkg;
>      } elsif ($spoolname eq 'task') {
>         return bless {
>             'spoolname'  => 'task',
>             'generator'  => 'Task',
>             'storagemgr' => 'Backend::File',
>         } => $pkg;
>      } elsif (...) {
That seems really a bad idea to integrate application logic directly in 
the low-level objects. In MVC model, this is mixing the model and the 
controller. The day you need another spool, you should not have to 
modify the Spool class.

Here is a better approach IMHO:

package Sympa::Spool;

sub new {
    my ($class, %params) = @_;

    # check parameters presence and type

    my $self = {
        name    => $params{name},
        content => $params{content}
        storage => $params{storage}
    };
    bless $self, $class;
    return $self;
}

package main;

my $incoming_spool = Sympa::Spool->new(
    name    => 'incoming',
    content => 'Sympa::Message',
    storage => 'Sympa::Spool::Storage::File'
);
my $pending_spool = Sympa::Spool->new(
    name    => 'pending',
    content => 'Sympa::Message::Key',
    storage => 'Sympa::Spool::Storage::SQL'
);
my $task_spool = Sympa::Spool->new(
    name    => 'task',
    content => 'Sympa::Task',
    storage => 'Sympa::Spool::Backend::File'
);

> sub get_message {
>      my $self      = shift;
>      my $selection = shift;
>
>      # fetch() gets a record from physical storage and extract it to
>      # hashref.
>      # new() generates object from hashref.
>      return $self->{'generator'}->new(
>         $self->{'storagemgr'}->fetch($self->{'spoolname'}, $selection)
>      );
> }
>
> sub store {
>      my $self   = shift;
>      my $object = shift;
>
>      # serialize() is inverse function of new().  It extracts object
>      # to hashref.
>      # add() adds a record represented by hashref to physical storage.
>      $self->{'storagemgr'}->add(
>         $self->{'spoolname'},
>         $self->{'generator'}->serialize($object)
>      );
> }
>
> 1;
> ------------ >8 ---------------- >8 ---------------- >8 ------------
>
> It's just an idea.
I like the idea of just storing the content class name in the spool 
object, so as to call new() and serialize() on it. This avoid the need 
of a dedicated Spool class hierarchy just to handle the various kind of 
content (message, task, etc...), so we just have to deal with the 
various kind of storage backend (file, sql, ...)

So, I'd suggest to not use delegation pattern for the backend, and use 
the following class hierarchy:

Sympa::Spool       -> the basic abstract class
Sympa::Spool::File -> the file-based spool class
Sympa::Spool::SQL  -> the SQL-based spool class

My previous exemple is now:

my $incoming_spool = Sympa::Spool::File->new(
    name    => 'incoming',
    content => 'Sympa::Message',
);
my $pending_spool = Sympa::Spool::SQL->new(
    name    => 'pending',
    content => 'Sympa::Message::Key',
);
my $task_spool = Sympa::Spool::File->new(
    name    => 'task',
    content => 'Sympa::Task',
);
--
Guillaume Rousse