Developers of Sympa

[IKEDA Soji <address@concealed>]

Guillaume,

On Fri, 31 May 2013 14:58:00 +0200
Guillaume Rousse <address@concealed> wrote:

> 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',
> );

I think interfaces of each spool is independent from its backend.
For example by my idea,

  use Spool;
  ...
  $incoming_spool = Spool->new('incoming');
  $message = $incoming_spool->get_message();
  ...
  $pending_spool = Spool->new('pending');
  $pending_spool->store($message);
  ...
  $message = $pending_spool->get_message();
  ...

"incoming" spool may use "msg" storage (directory) by "File"
backend, "pending" spool may use "mod" storage (table rows) by
"SQL" backend, ... or they may not.  Details were given by Spool
package only at once.

Regards,

--- Soji


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