[Core] Clean public API of components

[alxbilger]

The usual pattern of component classes is the following:

• A generic method is defined in the base class of the component. Since it is generic, it does not know the type of the DataTypes (it's not templated). Instead, it acts on VecId.
• In a second base class, inheriting from the first one, the DataTypes is known (the class is templated). Therefore, the methods can be defined to work on the right type.
• In this class, the generic method calls the template-specific one.

IMO, the template-specific method should not be public. In this PR, they are moved to protected. I think also, that the generic methods can be final, but this is breaking, so I am not sure that it is a good idea to keep the final keyword. The PR starts with the final keyword to study the consequence on the CI.

Those changes force the user to call only the generic methods. I took the opportunity to add a check on the component state in the generic methods. If the final keyword is used, it would ensure that the component state is called before each call of the public API.

---

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).

[bakpaul]

I call that 'un doigt dans l'engrenage', nice !

I totally agree on the fact that those methods should be final otherwise the 'delegate' pattern used here makes no sense because we cannot trust it for child class...

But then I don't fully agree on the protected part : it might become cumbersome to test them in unit test with such protection

It might be good to understand why those base methods where overridden instead of the "delegate' one, maybe old artifact on un-refactored components ?

[fredroy]

It seems to be part of an other branch/PR, no ?

[alxbilger]

no it's not. I use it in the generic methods, before calling the template-specific methods

[alxbilger]

@bakpaul

I call that 'un doigt dans l'engrenage', nice !

Yes...

But then I don't fully agree on the protected part : it might become cumbersome to test them in unit test with such protection

It is exactly what happens now. However, I believe that the tests should not drive the design of a class. Today, I don't have a solution to test the protected methods (even if it's theoretically possible). So I am not against keeping them public.

[damienmarchal]

Hello,

Thanks for the PR. I fully agree on generalizing more the 'delegate' pattern and this is a proper usecase.

• About the use of final (on the public api) and private (on the delegated part): both are the way to go.

• About testing privates methods:
  In tests, shouldn't we prefer implementing tests using the generic interface, so by calling BaseForceField::addKToMatrix() on instance of BeamFEMForceField instead of writing tests for the specific BeamFEMFOrceFIeld::doAddKToMatrix(). I see a lot of added values of writing tests using the public API while testing private method of a delegated patterns are not obvious to me but.
  That's said if BeamFEMForceFIeld::doAddKToMatrix() is private and final, we can probably make it "testable" by having a friend class Test;

In the code base we are making the use of this pattern obvious by using a specific naming scheme (XXXX -> doXXX).
The intention what to make visually clear what part was part of the public API and what part was using the "delegate" pattern.
As in:

BaseData::beginEditVoidPtr() 
Data<T>::doBeginEditVoidPtr() 

I think it would be worth to stick to the scheme for the following reasons:

• it de-ambiguate the purpose of each of the function. With addKToMatrix(blabla) and addKToMapping(otherblabla) nothing indicates, at first glance, that the two are in fact not equivalent, and ones has to refer to code comment or documentation (if any) to get that a very specific design pattern is involved. Having addKToMatrix(blablag) and doAddKToMapping(otherblablab) makes it in the contrary cristal clear).
• using the "do" prefix also have the advantage that to the pattern is possible when the method's attributes are the same (In the PR using the same names works because the de-ambiguation is implicitly done through the different BaseMatrix and MechanicalParams).
• more consistant codebase

Of course, this means renaming the the addKToMatrix but in that matter, earlier is better and as the PR is breaking... this may be ok ;)