Refactor init() to replace the call super pattern by the delegation one

[damienmarchal]

Currently in Sofa we are using a lot the "call super" design pattern as in:

class BaseObject
{
public:
     virtual void init()
     { 
         checksDataFields(); 
     }

     virtual void draw()  {} 
};
class MyObject : public BaseObject
{
public:
     void init() override
     {
             componentState = Invalid
             BaseObject::init()
             /// ..........  do here the object specific initialization.
             if(error)
             {
                 emitMessage()
                 componentState = Invalid
                 return;
             }    
              componentState = Valid
     }
     
     void draw() override 
     {
          if(componentState != valid)
              return
          /// ......... do here the drawing    
     } 
}

This pattern used for init() and draw() has several drawbacks. The most problematic is that it delegates to the child classes the role of guaranting the correctness (ie: the child musn't forget to call BaseObject::init in MyObject::init). Doing so, at the right location(begin, end fo the init) also force the child to "know" what the super::init implementation is doing (to not duplicate) and that the implementation of BaseObject::init() will not change.

This kind of pattern can be replace with the "delegate pattern" (not sure this is its official name). Which leads to:

class BaseObject
{
public:
     void init()
     { 
           checksDataFields(); 
           doBaseObjectInit();
           setComponentStateOnError();
     }
     
    void draw()
    {
           if(componentState != valid)
              return
           doBaseObjectDraw(); 
    }
private:
     virtual void doBaseObjectInit() {}
     virtual void doBaseObjectDraw() {}
};

class MyObject : public BaseObject
{
public:
     void doBaseObjectInit() override final
     {
             /// do here the object specific initialization without bothering of what your parent is doing. 
     }
     
    void doBaseObjectDraw() override final
    {
            /// do here the object specific drawing without bothering of what your parent is doing. 
    }
}

The latter pattern allows to implement a general behavior in a parent class and be sure this behavior is never broken up by the child.

In the PR i tried it to see what would be the change have a look at. The change in BaseObject is at commit https://github.com/sofa-framework/sofa/commit/5dd2761c7920775e81672cc9a5a12bcaf27b9ee2

Suggestion and improvement welcom :)

EDIT: yes I'm perfectly aware this is a radical refactoring if we are going to deploy it on the whole code base ;)

---

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).

---

Reviewers will merge this pull-request only if

• it builds with SUCCESS for all platforms on the CI.
• it does not generate new warnings.
• it does not generate new unit test failures.
• it does not generate new scene test failures.
• it does not break API compatibility.
• it is more than 1 week old (or has fast-merge label).

[alxbilger]

We already talked about that, and indeed you explained very well the nature of the problem and how to solve it. I don't think of a better approach.

It is important to note that this pattern needs to be implemented again for classes, derived from BaseObject, and base class themselves. Look at BarycentricMapping, derived from Mapping, for example: it calls Inherit1::doBaseObjectInit, which is actually the same problem that you described. Here also it should be replaced by a "delegate" pattern.

Currently, the fact that init() is no longer virtual is breaking. I had the same problem in https://github.com/sofa-framework/sofa/pull/3242. Do you have an idea to guide the developers in the update of their code?

Finally, I don't like doBaseObjectInit, and it is a problem of this pattern: we need to invent new names for delegate methods even though they are similar to the initial method.

[damienmarchal]

We discussed a lot about it...but we never did a PR on that topic probably fearing the refactoring work and the amount of workload it would generates.

It is important to note that this pattern needs to be implemented again for classes, derived from BaseObject, and base class themselves. Look at BarycentricMapping, derived from Mapping, for example: it calls Inherit1::doBaseObjectInit, which is actually the same problem that you described. Here also it should be replaced by a "delegate" pattern.

Yes, all call-super need to be fixed that way (which I didn't in the PR), I will not do that unless it is clear we are really going to do such a breaking change in the sofa code base ;)

Currently, the fact that init() is no longer virtual is breaking. I had the same problem in https://github.com/sofa-framework/sofa/pull/3242. Do you have an idea to guide the developers in the update of their code?

I'm not sure I get the point

EDIT: (....I'm reading 3242 to see if I understand).
EDIT2: the easy non breaking strategy is to use new names for the base virtual function eg: newInit (not sure we want that). (good code use override so if we keep the old name the code will breaks will be detected at compile time) but this is probably not enough...

Finally, I don't like doBaseObjectInit, and it is a problem of this pattern: we need to invent new names for delegate methods even though they are similar to the initial method.

Instead of "inventing" new names for delegates methods I strongly advocate to rely on single and clear name scheme.

Currently I'm doing:

do _ ClassThatDelegatesToYou _ MethodDelegated

so BaseObject::init() become doBaseObjectInit() (which I interpret as "do" the init as requested by BaseObject).

The underlying idea of the naming scheme is that with a first sight we get a lot of information... the (do) show the intention of the code and the pattern used, while the class's name indicates where to look at to get documentation on what the "methodDelegated" should do (to know contract the child has to fulfill).

The advantage I see with that naming scheme is that it works with chain of delegation. So the code should look like:

BaseObject::init()   /// non virtual 
{
     // I do the stuff shared by all child 
     checkWhatever();
     
     // ask my child to do their specific init, 
     doBaseObjectInit()
}

BarycentricMapping::doBaseObjectInit()  final
{
     // I do the stuff shared by all of my child 
     checkWhatever();
     
     // ask my child to do their specific init, 
     doBarycentricMappingInit();
}

MyChildOfBarycentricMapping::doBarycentricMapping() final
{
     // ask my child to do their specific init, 
}

This long naming scheme also has some good properties in case of multiple in-heritance, as it avoid colliding delegate function's name.

Of course, any other naming scheme suggestion is welcomed.

[damienmarchal]

[ci-build]

[epernod]

this remind me an old aborted try in this PR: https://github.com/sofa-framework/sofa/pull/1506

[damienmarchal]

@hugtalbot you removed the to discuss at dev meeting tag... but do we really discussed about that ?

[bakpaul]

Hi @damienmarchal, what is the state of advancement of this pr ? I know that there where some discussions on the subject in the last STC.