szaszm commented on a change in pull request #958:
URL: https://github.com/apache/nifi-minifi-cpp/pull/958#discussion_r542450733
##########
File path: libminifi/include/utils/TimeUtil.h
##########
@@ -53,6 +53,29 @@ inline uint64_t getTimeNano() {
return
std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::system_clock::now().time_since_epoch()).count();
}
+/**
+ * Mockable clock classes
+ */
+class Clock {
+ public:
+ virtual ~Clock() = default;
+ virtual std::chrono::milliseconds timeSinceEpoch() const = 0;
Review comment:
Currently I see the `Clock` class hierarchy as just related functions in
disguise. This change would allow us to get rid of the classes and replace them
with free functions (e.g. `get_time`, `get_steady_clock_time` and a lambda with
a captured local for the manual case), which is closer to the truth IMO.
I think functions are simpler than classes (one operation vs multiple
operations + data), so for my code, I would opt for that approach, but I'm
totally fine with you not sharing my viewpoint and keeping the code as is.
For the readability argument, I'm arguing that the usage we describe is
retrieving the time, and that can be described by naming the functions and the
type-erased member something along the lines of `get_time_`.
----------------------------------------------------------------
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]
