FairRoot
FairRoot copied to clipboard
About getter functions
This issue is dedicated to discuss how we should write (or improve) getter functions in FairRoot.
From the discussion with @dennisklein in PR #1254, we found it's very inconvenient to always check the pointer from getter functions to prevent nullptr dereferencing.
Here are my opinions regarding this issue:
-
Prefer
const auto&
whenever possible. The biggest problem of returningauto&
or a pointer is it breaks one of the most important principles in OOP design: encapsulation. This principle basically means the resource/data inside an object should be hidden for the change but open for the read. If a user wants to make a change, change it through interfaces. And in some rare cases where the user needs to make a lot of changes, then change the data via a setter. In principle, a getter should only be used for reading, not for changing in an OOP class. (Of course, this does not apply to unique_ptr as it's not an OOP class.) -
Provide one single getter function for a resource (usually by a class that owns it). We have
GetSink
fromFairRun
and we also have that fromFairRootManager
. RunID is another mess. We can get runID fromFairRun
,FairRootManager
,FairSource
,FairEventHeader
(maybe there are more). And users can get direct access to all these classes. Which class should I get the runID? Are they all the same? -
Prefer value for the member variable.
FairRun
ownsFairRuntimeDb
as it deletes the object in the destructor. So it's better to own it as a value not as a pointer. With this, there is no need to check whether it's nullptr or not.
Surely there are still many cases where we have to return a raw pointer and have a check on it. By no means should this way be always prevented. But with a better design, we could keep it to a minimum.
I'm certainly not a C++ or an OOP expert. If someone disagrees with my points or has some better ideas, please comment below. @janmayer @klenze @dennisklein