robotology / blockfactory

A tiny framework to wrap algorithms for dataflow programming
https://robotology.github.io/blockfactory
GNU Lesser General Public License v2.1
42 stars 16 forks source link

Polished and improved SimulinkBlockInformation #23

Closed diegoferigo closed 5 years ago

diegoferigo commented 5 years ago

I want to change the following methods in the BlockInformation interface:

virtual blockfactory::core::InputSignalPtr
    getInputPortSignal(const PortIndex idx, const VectorSize size = -1) const = 0;
virtual blockfactory::core::OutputSignalPtr
    getOutputPortSignal(const PortIndex idx, const VectorSize size = -1) const = 0;

and remove the size parameter. However, since this will affect also the SimulinkCoder implementation, I will open a new PR with also the rationale about it.

diegoferigo commented 5 years ago

Thanks @traversaro, feel free to send it to me if you find your comment by chance. Usually I never split the Impl class from the main one, if it happens often it means that it might be worth to create another class standing on its own (and I mean not being the private of another). In this case I don't see any possibility of code reuse of this small wrapper around the Simulink APIs, and I preferred to improve the readability of the SimulinkBlockInformation class by moving them out entirely, providing a sort of isolation.

traversaro commented 5 years ago

Thanks @traversaro, feel free to send it to me if you find your comment by chance. Usually I never split the Impl class from the main one, if it happens often it means that it might be worth to create another class standing on its own (and I mean not being the private of another).

Yes, my comment was basically this. In particular, in the past we had maintainability problem once logic was split between the actual class and the pimpl class (in the sense: if I had new logic to add, I add it in the main class or in the pimpl?), for this reason I always recommend to just put members in the pimpl, but in this case it make sense. if I would probably call the resulting class in a different way, even if it is just a private class, for example "SimulinkBlockInformationHelper", to make clear that is not a classical pimpl, but this are just details.

diegoferigo commented 5 years ago

Thanks for the feedback. For the time being let's merge this PR as it is since changing the class name would touch too many parts of file and it would be worth having a separate PR.