Templatizing a Timer class

Question

I'm making a timer class in C++. This is the overview:

#include <chrono>
namespace ktp {

class Timer {

  using Clock     = std::chrono::high_resolution_clock;
  using TimePoint = std::chrono::high_resolution_clock::time_point;

 public:

  Timer(bool start = false) {
    if (start) Timer::start();
  }

  void pause() {
    if (m_started && !m_paused) {
      m_paused_time = now();
      m_paused = true;
    }
  }

  auto restart() {
    const auto time {milliseconds()};
    stop();
    start();
    return time;
  }

  void resume() {
    if (m_started && m_paused) {
      // we substract the total duration of the pause
      m_started_time = now() - (m_paused_time - m_started_time);
      m_paused = false;
    }
  }

  void start() {
    m_started_time = now();
    m_paused_time = {};
    m_started = true;
    m_stoped = false;
    m_paused = false;
  }

  void stop() {
    m_started = false;
    m_paused = false;
    m_stoped = true;
    m_started_time = {};
    m_paused_time = {};
  }

  // THIS BELOW IS WHAT I DON'T LIKE

  const std::chrono::milliseconds milliseconds() const {
    if (m_started) {
      if (m_paused) {
        return std::chrono::duration_cast<std::chrono::milliseconds>(m_paused_time - m_started_time);
      } else { // started and running
        return std::chrono::duration_cast<std::chrono::milliseconds>(now() - m_started_time);
      }
    }
    return std::chrono::milliseconds {};
  }

  const std::chrono::seconds seconds() const { 
    // same as milliseconds but with seconds
  }

  // more of the same for nanoseconds, minutes, etc...

 private:

  static const TimePoint now() { return Clock::now(); }

  static inline const TimePoint s_initialization_time {now()};
  TimePoint m_paused_time {};
  TimePoint m_started_time {};
  bool m_paused {false};
  bool m_started {false};
  bool m_stoped {false};
};

} // end namespace ktp

As you can see, I've to write a bunch of functions that are the same, it justs changes the type. So I decided to make a private template like this:

  template<typename T>
  const T duration(T) const {
    if (m_started) {
      if (m_paused) {
        return std::chrono::duration_cast<T>(m_paused_time - m_started_time);
      } else { // started and running
        return std::chrono::duration_cast<T>(now() - m_started_time);
      }
    }
    return T {};
  }

Which changes the milliseconds() and the other time returning functions to this:

const std::chrono::milliseconds milliseconds() const {
    return duration(std::chrono::milliseconds {}); // this object
  }

But I feel that creating an object and passing it as an argument just to know it's type maybe is not the best option. The code works, but I'm wondering if anyone knows a better solution.

Answer 1

Let the caller convert durations

You are right that having milliseconds() and seconds() member functions is not nice, and you should get rid of them. However, using a template like you suggested is also not the answer. The right answer is to return a Clock::duration value, and let the caller convert that to whatever time unit they want. This reduces the responsibilities of your class, and actually makes it more flexible as well.

I suggest this to replace the offending functions:

using Duration = Clock::duration;
...
Duration get_elapsed() {
    if (m_started) {
        if (m_paused) {
            return m_paused_time - m_started_time;
        } else { // started and running
            return now() - m_started_time;
        }
    }

    return {};
}

Answer 2

1. Time warp

Anything but std::chrono::steady_clock does not guarantee being monotonic and ticks having more-or-less constant time between them as time moves forward. On libstdc++, it does syscalls with CLOCK_MONOTONIC. std::chrono::high_resolution_clock gives current time point with high precision by calling CLOCK_REALTIME. tl;dr use steady clock for timing time intervals, while high resolution clock should be used to get precise points in time that then are interpreted somehow.

2. Unclear usage

I have not seen an example usage. If this is supposed to be a part of benchmarking framework, it should have other facilities like optimization prevention, statistical functions and input generation. If it is just a oneshot timer that is removed before release, I would just get a start time at construction and print elapsed time on destruction:

struct timer {
    using clock = std::chrono::steady_clock;
    clock::time_point start_time;
    std::string_view prefix_message;
    timer(std::string_view prefix_message) noexcept:
        start_time(clock::now()),
        prefix_message(prefix_message)
        {}

    ~timer() {
        auto end_time = clock::now();
        std::cout << prefix_message << ' ' << std::chrono::duration_cast<std::chrono::microseconds>(end_time - start_time).count() << " microseconds\n";
    }
};

The interface presented in OP looks a bit hard to use and easy to get wrong, while the above can be used like this:

void foo(ArgType arg) {
    arg = init();
    { // start of benchmark section
        timer t("timing foo without init");
        bar(arg);
        /*some more code*/
    } // end of benchmark section, destruction will correctly print the result, 
      // also it is guaranteed to be the last one destructed, barring speculative execution
}

Answer 3

I will not comment on the code itself, there is already an answer discussing the issues with the chosen clock. Instead, I want to discuss the API.

The API for your class is not easy to learn to use. Several factors play a role:

1. Lack of documentation. I would prefer to see some comments at the top of each function (e.g. Doxygen style) that explain what each function does and how to use them. Instead, I need to read the code to learn what each function does.

2. Deviation from expectations. The name "timer" could be used in the way you use it, but is more often used for a device that counts down. You set a timer to a specific amount of time, and a bell goes off when that amount of time has elapsed. A "stopwatch" is a device used in e.g. sports to measure how long something takes (such as a lap around the course).

3. Redundant functions. Some can only be used under certain circumstances (which you test for). A stopwatch usually has a start, a stop and a reset button. If you follow that design, the use of the functions will be more obvious, you will have fewer functions, and the functions will be simpler. "Start" makes the clock count up, from wherever it was at the time. "Stop" stops the counting. "Reset" sets the count to zero. This is all you really need to implement all the functionality your class has (plus the "read current time" function of course). With this design you have only one status bit ("running" vs "stopped"), rather than tree (m_started, m_paused, and m_stoped).

I agree with G. Sliepen that the "read current time" function could return a Clock::duration. An alternative would be to just always return the time in seconds, and let the user convert to milliseconds if they wish, the conversion is trivial.