[Freeipa-devel] [PATCH] 0270-removing-setters-setup-and-init

[Endi Sukma Dewata]

On 7/25/2011 11:56 AM, Adam Young wrote:
>>  30. The IPA.spacer_widget is used to create spacing for layout.
>>  Widgets should not be used this way because it diverges from the
>>  original design. Widgets are supposed to correspond to attribute. It
>>  has a name (required for storing in a map), metadata, load/save/undo
>>  methods, none of which is relevant for spacer. Also, it only handles
>>  a narrow case for creating a space between 2 fields which I think
>>  better be handled using sections. This should be addressed in a
>>  separate patch.
>
> Agreed. Fixing this will be easier to do on top of this patch.

I think we should remove IPA.spacer_widget and leave the 
IPA.dnszone_adder_dialog unchanged for now. The current code, although 
it might not be very concise, is done correctly and does produce the 
desired layout (label on the right of the checkbox and a space below 
it). The IPA.spacer_widget on the other hand violates the design and 
still doesn't produce all the intended layout. Also, chances are the 
priority for fixing this afterward will be low. Suppose we change the 
IPA.widget to verify that all widgets have a name, this code will fail 
immediately.

>> 31. The create() should only be used to create the visual elements.
>> Creating fields, columns, etc. should not be done inside create()
>> because we might decide to destroy the HTML elements of hidden facets
>> and recreate it again. In that case the fields will be duplicated.
>> Also, by delaying the creation of the fields it makes the object
>> incomplete after creation, which is the reason for removing init().

> Agreed, but I'm going to postpone fixing this until after this patch.
> Can you document where you see this as a problem?

I think this should be done in this patch because moving the code from 
init() into create() changes how the code is executed (creating 
incomplete objects, changing the assumptions). I've listed the code that 
is affected in the new issues below.

>> 32. This is an existing problem but it's worsening without init().
>> Since there is no class constructor, the initialization code is
>> scattered throughout the class, making it difficult to maintain. Some
>> code needs to be written in certain position, but in general should we
>> move the initialization code to the bottom of the class (to make sure
>> all methods are already defined)? Or should init() be used as a
>> constructor and called at the bottom of the class?

> Instead of making a generic init function call, I think making smaller
> (potentially reusable) functions for related chunks of code, and then
> calling all of them at the bottom of the "constructor" makes the most
> sense. I did a little bit of this in this patch for the various sections
> of rules details facets, and I think it will have the desired effect.

Let me clarify the problem, in some of our code the initialization code 
is intermixed with attribute declarations and function definitions which 
makes it hard to understand and maintain.

IPA.some_class = function() {
     ... <initialization code> ...
     ... <attribute declaration> ...
     ... <initialization code> ...
     ... <function definition> ...
     ... <initialization code> ...
};

It's not always clear what attributes the class has or when they are 
initialized. In many cases function invocation looks similar to function 
definition, so you'd have to carefully read it. Sometimes there are 
unexpected function invocation in the middle of the class definition. 
Removing the init() and moving the initialization code into the class 
definition (different from issue #31) exacerbates the problem further 
because it blurs the boundary between function definition and invocation.

The solution is to move the initialization code into a single contiguous 
location and clearly mark it as constructor.

One option is to wrap the initialization code in a function and call it 
from the bottom of the class.

IPA.some_class = function() {

     ... <attribute declaration> ...
     ... <attribute declaration> ...

     that.init = function() { // Constructor
        ... <initialization code> ...
        ... <initialization code> ...
     };

     ... <function definition> ...
     ... <function definition> ...

     that.init(); // Call constructor
};

The new init() is different from the old init() because it's executed 
before the new instance is returned to the caller.

The other option is to move the code to the bottom of the class and mark 
the area as constructor.

IPA.some_class = function() {

     ... <attribute declaration> ...
     ... <attribute declaration> ...

     ... <function definition> ...
     ... <function definition> ...

     // Constructor
     ... <initialization code> ...
     ... <initialization code> ...
};

I prefer to use the wrapper because it mimics the constructor definition 
in proper OO languages. Also we can keep the initialization code at the 
beginning of the class which is more intuitive and reduces the changes 
to the code. But either way will work just fine.

We can still refactor the initialization code into reusable functions 
like you described. It should look like this:

IPA.some_class = function() {

     ... <attribute declaration> ...
     ... <attribute declaration> ...

     that.init = function() {
        that.func1();
        that.func2();
     };

     that.func1 = function() { ... <initialization code> ... };
     that.func2 = function() { ... <initialization code> ... };

     ... <function definition> ...
     ... <function definition> ...

     that.init();
};

But not like this:

IPA.some_class = function() {

     ... <attribute declaration> ...
     ... <attribute declaration> ...

     that.func1 = function() { ... <initialization code> ... };
     that.func1();

     ... <function definition> ...

     that.func2 = function() { ... <initialization code> ... };

     ... <function definition> ...

     that.func2();
};

Here are the itemized list of issues:

41. The radio buttons in the 'As Whom' section in sudo rule section are 
missing the labels. It should show the doc attributes of the 
ipasudorunasusercategory and ipasudorunasgroupcategory.

42. The code in widget.js:1124-1128 can be replaced with this:

    that.entity_name = spec.entity ? spec.entity.name : spec.entity_name;

In general attribute declarations should be 1 liner. If it takes more 
than 1 line it should be done in the constructor.

43. The initialization code (set_param_info) in IPA.widget 
(widget.js:80) was originally in init(). It should be moved into the 
constructor.

44. The initialization code in IPA.column (widget.js:1135-1147) was 
originally in init(). It should be moved into the constructor.

45. The initialization code (field creation) in IPA.target_section 
(aci.js:401-433) should be moved into the constructor. This is an 
existing issue so it can be fixed separately, but this is just to show 
the problem we are dealing with.

46. The attribute declaration (target_types) in IPA.target_section 
(aci.js:458-592) should be moved to the beginning of the class. This is 
also an existing issue.

47. The initialization code (option creation) in IPA.rights_widget 
(aci.js:366-369) was originally in init(). It should be moved into the 
constructor.

48. The initialization code (button creation) in IPA.add_dialog 
(add.js:48-93) was moved from init() to create(). It should be moved 
into the constructor.

49. The initialization code (column creation) in 
IPA.association_adder_dialog (association.js:157-165) was moved from 
init() to create(). It should be moved into the constructor.

50. The attribute declaration (NORMAL_HEIGHT and WITH_EXTERNAL_HEIGHT) 
in IPA.adder_dialog (dialog.js:341-342) should be moved to the beginning 
of the class.

51. The initialization code (spec validation and table creation) in 
IPA.adder_dialog (dialog.js:305-311 and 345-367) was moved from init() 
to create(). It should be moved into the constructor.

52. The initialization code (table creation) in IPA.sudo.options_section 
(sudo.js:603-638) was originally in init(). It should be moved into the 
constructor.

53. The initialization code (field creation) in 
IPA.sudo.rule_details_command_section (sudo.js:784-823) was originally 
in init(). It should be moved into the constructor.

54. The initialization code (field creation) in 
IPA.sudo.rule_details_runas_section (sudo.js:967-1020) was moved from 
init() to create(). It should be moved into the constructor.

55. The initialization code (column creation) in 
IPA.sudo.rule_association_adder_dialog (sudo.js:1185-1208) were moved 
from init() into create(). It should be moved into the constructor.

There maybe more of similar problems. It's a long patch...

-- 
Endi S. Dewata