szaszm commented on a change in pull request #716: MINIFICPP-1127 - Provenance
repo performance should be improved
URL: https://github.com/apache/nifi-minifi-cpp/pull/716#discussion_r374657478
##########
File path: libminifi/include/core/Repository.h
##########
@@ -85,6 +85,10 @@ class Repository : public virtual
core::SerializableComponent, public core::Trac
virtual void flush();
+ virtual void printStats() {
+ return;
+ }
+
Review comment:
Introducing an empty function to a base class is a smell. In this case,
`printStats` doesn't need to be `virtual` or even present in `Repository`.
From the standpoint of contracts:
By introducing `printStats` to `Repository`, `Repository` now looks like
something that can print stats of itself. The reality is that only
`ProvenanceRepository` (and potentially later a select other few derived
classes) can offer this functionality. In all other cases, this is a broken
promise.
From the OOP standpoint:
`printStats` is a method specific to `ProvenanceRepository` but has leaked
to `Repository`, breaking encapsulation.
From a performance standpoint:
Why pay for virtual dispatch when in all of the callers of `printStats`, the
static and the dynamic types of the object match? We could even go as far as
removing the function declaration from the headers altogether and giving it
`static` linkage, therefore giving the compiler the freedom to throw the
implementation away (except for 1 inlined instance) to reduce code bloat.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
