Developers of Sympa

[Guillaume Rousse <address@concealed>]

Hello.

I'm a bit concerned by the way object attributes are handled in the 6.2 
branch, and I think getting a consensus here would help solving other 
issues. I'm more and more convinced than the current twisted 
relationships between Site_r,  Site, List and Robot classes is just 
motivated by implementation reasons, rather than modeling purposes.

If I understand correctly what has already been merged in cleanup branch:
- a robot object has two kind of properties:
 + a list defined in Sympa::Site namespace (etc, home, name), called 
'attributes'
 + a list defined in Sympa::Site namespace (blacklist, ...), plus 
another list in confdef namespace, called 'parameters'
For each of those properties, a single read/write accessor is 
automatically generated through AUTOLOAD mechanism

- a site class has a single type of properties, called 'attributes', 
defined in Sympa::Site namespace (auth_services, etc...). For each of 
those properties, a single read/write accessor is automatically 
generated through AUTOLOAD mechanism.

Here a my concerns.

First, we have a vocabulary issue: what is an object attribute ?
Here is is used as 'some kind of object property which is not a 
parameter', whereas for me, it is a plain synonym of 'object property'.
For me, everything with an accessor is an object attribute, wether its 
value comes from a configuration file, is internally generated, etc...

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)

Third, it clutters our API. I'm currently counting 17 public functions 
or methods in Sympa::Robot. Automatically generating methods for each 
element of confdef::params will add 258 additional methods...

Fourth, it breaks another good practice, which considers than each 
method name should be following an explicit naming rule, the more usual 
being 'action_object' (for instance "send_message", "execute_query', 
etc...). What does means $robot->main_menu_custom_button_3_target() ?

Moreover, those accessors being read only, they'd better get named 
'get_something()' than 'something()'. You don't need to be a genious to 
understand than there is something wrong in 
'$robot->get_home("/var/lib")', whereas '$robot->home("/var/lib")' is 
more ambiguous.

Fifth, it make use use AUTOLOAD, just to automatically generate those 
additional methods. AUTOLOAD was supposed to be part of solution, I 
think it is becoming the main problem in new code:
- useless complexity: AUTOALOAD method in Sympa::Site is unmaintainable, 
especially without related tests
- unwanted side-effect: DESTROY method has to be explicitely defined, 
even without content, just to avoid side-effect

So, my current proposal would be:
1) find another name for 'the kind of object property which is not a 
parameter', instead of attribute.
2) stop generating accessors dynamically, and stay with a single 
get_parameter() method. Eventually, another method could be needed if we 
really have two different type of attributes, and they need to be 
handled differently
3) find some way to enclose attributes definitions inside classes 
definitions

Also, the goal is to be consistent, both in our implementation, and in 
our API, across all our clases, not just Sympa::Robot and Sympa::List. A 
database object, has statically-defined get_type(), get_name(), 
get_host() and get_user() accessors, for instance. Why should we manage 
them differently other classes ?
--
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