leggedrobotics / ros_best_practices

Best practices, conventions, and tricks for ROS
https://rsl.ethz.ch/education-students/lectures/ros.html
BSD 3-Clause "New" or "Revised" License
1.49k stars 413 forks source link

Do not prevent subclassing your libraries. #23

Closed peci1 closed 4 years ago

tomlankhorst commented 4 years ago

Virtual functions have a non-zero performance impact which might be undesirable for a piece of code like an algorithm. A class might resort to composition to extend the functionality of Algorithm, it should then rely on the publicly exposed functions of Algorithm. I'm not a big fan of protected functions since functions should either be part of the interface (public) or not (private).

peci1 commented 4 years ago

You're right virtual functions might be costly if the function is called often. Maybe the code could have a non-virtual function OftenCalledFunction and a virtual NotSoOftenCalledFunction. But that decision is on you.

As for protected members - composition is sometimes inefficient. Imagine a class that has some data protected by a mutex. And it has public fuction getData() and private getDataNoLock(). If you want to extend the functionality and the latter function is private, you have no other way than to call the locking variant every time. Even though you might either pretty well know when locking is needed, or you might even hold the lock (if it's made protected, too). This is something composition has no answer for.

tomlankhorst commented 4 years ago

Thank you for your reply and suggestions.

I do agree with you that there are situations imaginable where composition is challenging.

However, say there exists a locking public function getData() and there is a non-locking variant getDataNoLock() in class A:

If you control both class A and the extending class B, you could also resort to friend declarations.

Now, I do consider adding an example where inheritance is more appropriate, but this algorithm is probably not it.

peci1 commented 4 years ago

You're right, there are always ways. Making functions protected is (from my point of view) more a safety feature "against" users of the class. Or in other words, there is "user API" (public functions) and "developer API" (protected functions). By using public and protected you can nicely tell these two apart.

Friend declarations are something that I really don't like, because they break all kinds of contracts. The only place where I like using them is testing. But even there I like to keep the base class "clean" of friend declarations, and in the test, I subclass it, and add the tests to be friends of this subclass. Again, when things are private, you can't do it this way and would have to clutter the base class with totally irrelevant friends.

But that's all just my viewpoint. I'm aware that some best practices go in the direction you present.

I support the idea of adding another example for inheritance and keeping algorithm as it is.