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

[Endi Sukma Dewata]

On 7/25/2011 8:59 PM, Adam Young wrote:
>> 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.

> Can't: the code you had working was based on the split of a couple of
> the functions, and now the pieces needed from the base class are not
> split out. Getting this to work again will require either significant
> code duplication or reworking of the DNS class, and neither is
> preferable to a short lived space_widget. The best we can do is remove
> the space_widget and not have any visual separation, and then do the
> visual separation in a different patch. I didn't want to change what you
> had working in this patch, but I had to do something to get it to work
> again.

Yes, let's remove the visual separation for now.

> For the rest of the review issues, I think I would prefer the other
> option: "move the code to the bottom of the class and mark the area as
> constructor." However sometimes some code needs to be done prior to the
> baseclass call:
>
>
> modify_spec(spec);
>
> var that = IPA.base_class(spec);

This is a normal issue that happens in other OO language too, for example:

public class GoldenRectangle extends Rectangle {
     public GoldenRectangle (int width) {
         super(width, 1.618 * width /* height */);
     }
}

However, the parameter modification should be kept to a minimum. Like in 
Java it cannot/should not invoke any methods before calling super(). If 
it can't be done that way, the class need to be redesigned (i.e. pass 
the parameter after creation).

> And then, in many places, we do
>
> that.create = function(container){...}
>
> If we moved the code to the bottom, we'd have to do
>
> IPA.somthing = function(spec){
> function create(container){}
> //constructor
> modify_spec(spec);
> var that = IPA.base_class(spec);
> that.create = create;
> }
>
> Which is probably a better coding practice, but would change a lot of code.

We don't need to change it like that. Even in the current code, the 
modify_spec cannot call public methods defined in 'that' because 'that' 
is not defined yet. It can only call private methods, which means the 
methods are already defined privately so we don't need to change it.

See the following example, we can convert this class:

IPA.something = function(spec) {

     ... modify spec ...

     var that = IPA.base_class(spec);

     that.init = function() {
         that.super_init();
          ... initialize instance ...
     };

     that.create = function(container) {
         ... create UI ...
     }

     return that;
}

into this with minimal changes:

IPA.something = function(spec) {

     ... modify spec ...

     var that = IPA.base_class(spec);

     that.init = function() {
         // don't call super_init()
         ... initialize instance ...
     };

     that.create = function(container) {
         ... create UI ...
     };

     that.init(); // call constructor

     return that;
}

or without init():

IPA.something = function(spec) {

     ... modify spec ...

     var that = IPA.base_class(spec);

     that.create = function(container) {
         ... create UI ...
     };

     // constructor

     ... initialize instance ...

     return that;
}

> our coding standard should probably be:
>
> NAMESPACE.function_name = function(spec){
>     ...
> }

Agree on the class structure. Many classes that we have are following 
that format already or pretty close.

> I'd like to get away from how we do super functions, too, but that is
> probably too ambitious. I feel like our current approach is fragile, and
> bucks the language, but the alternative is to use prototype inheritance,
> and again, that would change a lot of code, and the super method calling
> is not standardized anyway.

I think to fix 'super' properly we'd have to use one of the commonly 
used class frameworks and convert everything. I'd rather not reinvent 
the wheel which will only be used in this project. That's why it's 
important to keep the code as close as possible to regular OO design so 
we are not stuck with arcane code.

> I think that all the changes you have enumerated fall into the realm of
> "we need a standard but one has not been set yet." I'd like to get this
> patch in as is, and meanwhile we can hammer out what that standard
> should be.

As mentioned in ticket #1462, the first purpose of this patch is to 
remove the legacy init() which was written to delay initialization due 
to dependency on metadata. Since now the metadata is available much 
earlier, we can can create and initialize the object together. This is 
not about creating a new standard, but I think calling init() at the 
bottom of the class is the least invasive way to fix this. We should be 
able to do this without lowering the code readability.

The second purpose is to ensure the object created is valid (i.e. 
complete). As mentioned in issue #31, some of the initialization code is 
moved from init() to create() which is called even later than the old 
init(), so these objects will be created incomplete. There's a big risk 
changing the assumptions so close to release date.

The ticket also mentions about circular dependency issue. I've proposed 
a solution before by separating entity creation from facet/dialog 
creation, but we can fix this separately.

Issue #41 is a regression.

-- 
Endi S. Dewata