HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Follow-up: Timer utilizing std::future

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stdutilizingfuturetimerfollow

Problem

The previous code (as displayed in Timer utilizing std::future) is now:

```
#include
#include
#include
#include
#include
#include
#include

// PeriodicFunction
// =============================================================================

namespace Detail {

/// A functor to invoke a function periodically.
template
class PeriodicFunction
{
// Types
// =====

public:
typedef std::chrono::milliseconds period_type;
typedef Value value_type;

protected:
typedef std::function function_type;
typedef std::future future_type;

// Construction
// ============

protected:
/// Initialize with a function where the arguments are bounded to that function.
/// \SEE std::bind
template
PeriodicFunction(Callable&& callable, Arguments&&... arguments)
: m_function(std::bind(
std::move(callable),
std::forward(arguments)...)),
m_period(period_type::zero())
{}

public:
PeriodicFunction(const PeriodicFunction&) = delete;
PeriodicFunction& operator = (const PeriodicFunction&) = delete;

/// Calls stop.
~PeriodicFunction() { stop(); }

// Start/Stop
// ==========

/// True, if an invocation thread is active.
bool active() const noexcept { return m_thread.joinable(); }

/// Start an invocation thread and repeatedly invoke the function (in the given period)
/// after the given delay.
/// - If a previous invocation thread is active, no invocation of the function
/// takes place.
/// - After the first invocation of the function (at least one is done):
/// - If the period is or has become zero (due to a call to stop) the
/// invocation thread stops without any further invocation of the function.
/// - As long as an invocation of the function has not finished, the next
/// possible invocation is delayed by the given period.
/// \EXCEPTION All exceptions stop the invocation thread and the exceptio

Solution

Generally speaking, the code already seems really good. Therefore, I have nothing more than a few notes:

-
It is more of a matter of taste, but I would use some type aliases instead of the old typedef. The type aliases can be templated (contrary to typedef) and provide a syntax that is close to variable assignements (auto a = 89;) and namespace aliases (namespace foobar = foo::bar;). That means that you can write code which looks more consistent:

using period_type = std::chrono::milliseconds;
using value_type = Value;

using function_type = std::function;
using future_type = std::future;


-
C++ has special syntax to import type names from a base class without having to use a full typedef or type alias, you can use it to reduce the amount of code and make explicit your intent in PeriodicFunction:

using typename Base::period_type;
using typename Base::value_type;


-
typedef std::mutex mutex; seems useless and potentially confusing. I would drop the typedef and use std::mutex everywhere instead. It shouldn't hinder readability.

-
In their answer to your previous question, @ruds said that Timer::m_results should not be mutable, which is right, but I see no harm in having the corresponding std::mutex be mutable. It does not seem to be currently useful though.

-
Really no more than a tidbit, but namespaces are generally lowercase. Consider replacing Detail by detail.

-
Functions that return a bool tend to be more understandable when their name is more than just a name. For example, bool exception() {} makes me think that the function returns an std::exception or a derived class (which would be odd). Consider renaming it exception_occured instead. Also, consider renaming active to is_active.

Unfortunately, the standard library containers have a function empty while is_empty would have been a better name ("empty" somehow means "empty that container" to me); I understand that you keep this particular name for consistency.

Discussing what can be improved is good, but I also want to put a note about what you did right: tag dispatching with std::true_type and std::false_type is great. It helps to write simple and readable code and to avoid some std::enable_if and template boilerplate. You also correctly implemented perfect forwarding and used proper scoped locks. Kudos for that :)

Code Snippets

using period_type = std::chrono::milliseconds;
using value_type = Value;

using function_type = std::function<Value ()>;
using future_type = std::future<Value>;
using typename Base::period_type;
using typename Base::value_type;

Context

StackExchange Code Review Q#47347, answer score: 8

Revisions (0)

No revisions yet.