Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

It's often useful to be able to time code, for instance, to evaluate alternative approaches to the same problem. Because this is a thing I use frequently, I have created this Stopwatch templated class that I'd like for you all to review. Unlike some other recent code, this class is intended to be cross-platform and relies only on a working implementation of the std::chrono utilities in C++11.

Specifically, does the interface make sense? Could the code be improved? Also, I have thought it might be nice to provide some kind of a "units" function to return, say, a std::string containing "ms" if TimeT is set to std::chrono::milliseconds but ultimately decided against complicating the interface. I'd be open to comments on that decision as well.

stopwatch.h

#ifndef STOPWATCH_H
#define STOPWATCH_H
#include <chrono>

template<typename TimeT = std::chrono::microseconds, 
    typename ClockT=std::chrono::high_resolution_clock,
    typename DurationT=double>
class Stopwatch
{
private:
    std::chrono::time_point<ClockT> _start, _end;
public:
    Stopwatch();
    void start();
    DurationT restart();
    DurationT stop();
    DurationT elapsed() const;
};

template<typename TimeT, typename ClockT, typename DurationT>
Stopwatch<TimeT, ClockT, DurationT>::Stopwatch() 
{ 
    start(); 
}

template<typename TimeT, typename ClockT, typename DurationT>
void Stopwatch<TimeT, ClockT, DurationT>::start() 
{ 
    _start = _end = ClockT::now(); 
}

template<typename TimeT, typename ClockT, typename DurationT>
DurationT Stopwatch<TimeT, ClockT, DurationT>::stop() 
{ 
    _end = ClockT::now(); 
    return elapsed(); 
}

template<typename TimeT, typename ClockT, typename DurationT>
DurationT Stopwatch<TimeT, ClockT, DurationT>::restart()
{ 
    DurationT ret = stop();
    start();
    return ret;
}

template<typename TimeT, typename ClockT, typename DurationT>
DurationT Stopwatch<TimeT, ClockT, DurationT>::elapsed() const
{ 
    auto delta = std::chrono::duration_cast<TimeT>(_end-_start); 
    return delta.count(); 
}
#endif

stoptest.cpp

#include <iostream>
#include <cmath>
#include "stopwatch.h"

int main()
{
    const int iterations = 1000000;
    double sqrtsum = 0;
    Stopwatch<> sw;
    for (int i=0; i<iterations; ++i)
        sqrtsum += sqrt(i);
    std::cout << "calculated " << sqrtsum << " in " << sw.restart() << " us\n";
    sqrtsum = 0;
    for (int i=0; i<iterations; ++i)
        sqrtsum += pow(i,0.5);
    std::cout << "calculated " << sqrtsum << " in " << sw.stop() << " us\n";
}

A few words about usage

The goal was to create a minimal but complete interface. The constructor does nothing except calling start.

start sets both starting and ending times to the current underlying clock's now() and essentially functions as a reset (which was another name I considered for this function.)

elapsed does not modify the underlying Stopwatch and only returns the difference between the internally stored _end and _start.

stop returns the elapsed time between "now" and the internally stored _start and so can be used to measure several times all relative to the same start time.

restart does the same thing as stop except it also resets the start time.

share|improve this question
2  
You can use #pragma once instead of the define guard since, as far as I know, all compilers supporting C++11 also support this (unless you have some weird coding standards). It is safer and faster. –  Memleak Jul 25 at 17:25
    
What about exceptions. RAII is your friend for timing stuff generically. –  Loki Astari Jul 25 at 18:22
2  
@SebastianSG: #pragma once is not standard. Prefer include guards. Its more portable (and not difficult). –  Loki Astari Jul 25 at 18:30
    
@SebastianSG The purists who believe you should write your program to compile for legacy compilers would disagree. But I disagree with them. +1 –  Pharap Jul 25 at 19:39
    
@Pharap It's not a matter of legacy either. Include guards are much faster on GCC and Clang than they are for #pragma once. There was a post detailing this but I could not find it. –  Rapptz Jul 25 at 21:19

5 Answers 5

up vote 5 down vote accepted

Ergonomy

The restart method seems a bit unnatural: I wouldn't guess it returns the elapsed duration. When you look at this command as a sentence, it doesn't look right:

std::cout << "calculated " << sqrtsum << " in " << sw.restart() << " us\n";

How about renaming to delta() ?

Actually, I have a similar thing in Java. I have a delta() method that returns the elapsed time since the last call (or since start), and an elapsed() method that returns the total time since start. My start(), stop(), restart() methods don't return anything, and I feel it's right that way. Perhaps you can consider this alternative approach.

Coding style

I don't know what is the standard coding style in C++ world (and would appreciate a link!), but I think many of the standards I know in Java could be applied to C++ too, so I'll mention a few.

for (int i=0; i<iterations; ++i)
sqrtsum += pow(i,0.5);

It's recommended to use braces and indenting the block even for single statement fors, ifs, to avoid mistakes. Also, spaces around operators make the code more readable, like this:

for (int i = 0; i < iterations; ++i)
{
    sqrtsum += pow(i, 0.5);
}

(Btw I'm very happy to see you using ++i instead of i++, few people I know appreciate its significance!)

Modeling real life

I'm getting the feeling that the start and stop methods come from the notion of a stop watch in real life. In real life you first click start, then click stop, and read the elapsed time. To measure another thing, you click reset and then start again, and so on.

I think you can free yourself from a real life stop watch and think in terms of the functionality you're actually going to use:

  • Create stopwatch
  • Do something
  • Print elapsed time
  • Do something else
  • Print elapsed time
  • ...

The stop and restart actions just don't come into play here. When you think of what you actually want to do, the terms just don't pop up like that. You just want the elapsed time since the last "tick", again and again. start also isn't there, because it make sense to do that when you create the stopwatch, simply because there's nothing else really to do in the constructor of this object. Instead of doing nothing, you might as well just start it.

So I would rename restart to delta or similar (elapsed, or even tick), and omit start and stop completely. "stopwatch" might not be the best name for the class either. (I called mine "timer", but not sure that's much better.)

share|improve this answer
1  
C++ does not have a "standard coding style". ++i vs i++ is basically a micro-optimisation because the compiler will typically (well, always really) do it for you in loops. Outside of that this advice is good. –  Rapptz Jul 25 at 21:19
    
I thought about naming and what (if anything) to return. Note that stop, start and restart all imply a command, while elapsed is merely reporting, accurately reflecting actual implementation. That's the philosophy behind the existing names, but I'm open to suggestions for a better replacement set of names. Return values were based on observation of how I actually used the stopwatch. –  Edward Jul 25 at 22:48
    
That's the thing, since they imply a command, they don't fit intuitively in sentences like "calculated x in " << restart() << " seconds" or "calculated x in " << stop() << " seconds". Something that sounds reporting or evoking the notion of elapsed time would be more natural, and read like a sentence. –  janos Jul 25 at 22:53
    
@janos: In principle, I agree. I just couldn't think of anything better. Can you? –  Edward Jul 26 at 2:00
1  
I name it delta (as I already wrote it in the answer). A sentence like "calculated x in " << sw.delta() << " seconds" seems natural to me. Other ideas for the name: elapsed, tick. And I don't have start, stop, restart methods, as all I ever use is the .delta(). –  janos Jul 26 at 5:33

I would like this interface:

StopWatch    sw;

SomeTimeType   time = sw.time([](){
    for (int i=0; i<iterations; ++i)
         sqrtsum += sqrt(i);
});

std::cout << "It took: " << time << " Units\n";

I don't like the interface as it stands because you can call the functions in any order (which makes no sense). The interface should be designed so you can not call the functions in the wrong order. Which points at a design that does not involve explicit functions calls but a more declarative approach.

DurationT restart();        // Probably need to check the documentation on what that means.

DurationT elapsed() const;  // What happens if I call this on a currently running watch?
                            // I would still expect it to give me the current elapsed time
                            // but currently it return 0
share|improve this answer
    
Perhaps the interface is too flexible, but being able to accumulate time spent in multiple arbitrary slices of code is a useful feature. Also, I'd prefer to measure performance without affecting variable scopes, which lambda could do. –  200_success Jul 25 at 21:32
    
@200_success: One doesn't have to choose, really. I have a templated function called timeit that uses the Stopwatch class and accepts a function (including a lambda). I didn't post it because I found I never actually use it. –  Edward Jul 25 at 22:40
    
@200_success: OK I see the point about scope. –  Loki Astari Jul 26 at 16:17
    
@200_success: Multiple arbitrary slices are not really affected (because addition is not hard to do). –  Loki Astari Jul 26 at 16:17
    
@LokiAstari +1 "What happens if I call this on a currently running watch? I would still expect it to give me the current elapsed time but currently it returns 0" I also made this point, it's pretty serious. –  Pharap Jul 26 at 20:32

Maybe we all have different ideas on what a Stopwatch is. I see a Stopwatch as an object that measures the total_elapsed_time when the Stopwatch is active. That means functionally, a Stopwatch should have:

  1. start() - If inactive, define the start time for this period and make the Stopwatch active.
  2. stop() - If active, add the elapsed time during this duration period to the total elapsed time the Stopwatch was active, then set the Stopwatch as inactive. Whether it returns the elapsed time is up to you.
  3. split() or elapsed() - If inactive, return the total_elapsed_time the Stopwatch was active for. If active, calculate/update the total_elapsed_time the Stopwatch was active for and return it.
  4. reset(state) - The total_elapsed_time of the Stopwatch is reset to 0. State of the Stopwatch could be active or inactive (similar to the CTor).

An example of a Stopwatch class that follows this pattern of states used in production code is the System.Diagnostics.Stopwatch class in the .Net standard library.

I'm not a big fan of returning doubles for your DurationTs. Return your durations as TypeT objects to preserve the std::chrono type-safe property.

Also, I have thought it might be nice to provide some kind of a "units" function to return, say, a std::string containing "ms" if TimeT is set to std::chrono::milliseconds but ultimately decided against complicating the interface. I'd be open to comments on that decision as well.

A conversion operator (operator std::string()) that wraps elapsed() might work. Use TimeT::period::num and TimeT::period::den against a prefix look-up table (could just use the std::ratio as your key).

share|improve this answer
    
I like your prefix look-up table idea. I am experimenting with implementations of it now. –  Edward Jul 26 at 16:53
1  
I'd say the units thing should be delegated to a different class i.e. a TimeFormatter class. You could then provide different formatters for different time units and they'd all work through the same class/interface. +1 for Laying out the way a stopwatch should work, I totally agree that a stopwatch should work like that and the stopwatch presented does not. –  Pharap Jul 26 at 21:02
    
@Pharap Your idea is much better than mine. –  Snowhawk04 Jul 26 at 21:25

I have several things I'd like to mention, but to name a few:

  • Your stopwatch doesn't truly stop. What good is a stopwatch that doesn't stop?
  • start's body should be separated into two lines. Doing multiple assignments on a single line is bound to confuse at least one reader.
  • Stop and Restart should return either void or a bool indicating success.
  • elapsed should return a valid result even if the stopwatch is running, you should not have to stop the stopwatch just to be able to get an accurate reading. Think of a real stopwatch, you don't need to stop it to read how much time has elapsed, you get an updated value when you consciously check for one.
  • The stopwatch should have a way of telling what state it is in to prevent calls being made out of order by throwing exceptions or by ignoring calls being made out of order by only changing state when it is valid to do so (i.e. stop would only run its body if the stopwatch were in the running state).
share|improve this answer
    
The stopwatch does stop. But just like a mechanical stopwatch, it doesn't stop time itself (represented here by the underlying ClockT). –  Edward Jul 25 at 20:09
    
@Edward If I were to call stop, then record the result of elapsed, then call stop again, it would change the value of elapsed such that it would be as if the stopwatch had still been running. To truly make your stopwatch stop you should have a way of maintaining state, be it a bool indicating whether the stopwatch is running or an enum with values of Running and Stopped. –  Pharap Jul 26 at 1:28
    
Have you got a use case in mind in which you'd use that? –  Edward Jul 26 at 4:26
1  
Calculating load times of reading multiple files, but pausing during the processing of the data, or vice-versa, or any other situation where you need to pause the stopwatch. –  Snowhawk04 Jul 26 at 4:31
    
Sounds like pause() and unpause() make more sense... You could store the paused time and when unpausing just add the difference to the start time or keep track of a paused time total to subtract when calling elapsed(). I could see a lap() call being useful too, say if you wanted to use one stopwatch to record several parts of loading a file (i.e. reading from disk, parsing, inserting into database, etc.) with a lap() for each and the total time for processing the file from start to finish at the end. –  Jason Goemaat Jul 26 at 6:46

Every time I've needed a stopwatch, this is the sort of thing I've been after. No complications, just a simple start and stop function. I reckon you just document what the units are in, and people can do conversion themselves.

One thing that is convenient is something I saw in the .NET version of this, where you just have a static class that returns a running clock. Makes it even easier.

var stopwatch = System.Diagnostics.Stopwatch.StartNew();

// Do something

stopwatch.Stop();

Actually it looks like you're already doing that.

share|improve this answer
    
Note that the reason StartNew is a static method is because it is separate from the constructor, which avoids starting the Stopwatch. +1 for looking at an existing implementation as a reference though. –  Pharap Jul 26 at 22:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.