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

Static Observer Pattern in C++11

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

Problem

This is my first shot at implementing a fully static version of the Observer pattern in C++11. It is static in that all notifications are dispatched via CRTP, and therefore there is no virtual overhead. It also has the benefit of catching unimplemented notifications at compile time, by checking available overloads based on the Subject type. Its primary restriction is that all relevant observers must be known at compile time.

The motivation is to use this on an embedded system to help decouple systems while additionally providing better type safety. It appears that the initial output of this is exactly how I want it (zero cost) and the restriction of needing to know all observers in advance is reasonable given that the systems are well defined, and typically not dynamic. The platforms I work with typically do not allow malloc/new due to their ability to fail.

```
// static_observer header file
#include
#include

enum class EventType;

// CRTP Abstract Base class for implementing static subject.
// Example Subclass Usage -- Printing Observer:
// class Printer : public Observer {
// public:
// Printer() : timesTriggered_(0) {}
// template
// void OnNotify(Pressure &subject, EventType event) {
// std::cout GetID() class Observer {
public:
Observer() : obsID_(obsIDTracker_++) {}
template void OnNotifyImpl(T &subject, EventType event) {
static_cast(this)->OnNotify(subject, event);
}
int GetID() const { return obsID_; }
private:
int obsID_;
static int obsIDTracker_;
};
template int Observer::obsIDTracker_ = 0;

// Recursive helper structs for implementing calls to all observers held within subjects
template struct NotifyHelper {
static void NotifyImpl(T &subject, EventType event,
std::tuple &obs) {
std::get(obs).OnNotifyImpl(subject, event);
NotifyHelper::NotifyImpl(subject, event, obs);
}
};
template struct NotifyHelper {
static void NotifyImpl(T &subject, EventType event,

Solution

Personally (so you can take or leave this advice as this is just a personal pref) I find your style over compact and thus harder than necessary to read:

template  class Observer {

// I split this across two lines:
template 
class Observer {


Also:

class Observer {
public:
  Observer() : obsID_(obsIDTracker_++) {}
  template  void OnNotifyImpl(T &subject, EventType event) {
    static_cast(this)->OnNotify(subject, event);
  }
  int GetID() const { return obsID_; }
private:

// Everything is so squashed to the left:
// Bit more space to help readability never heart (and the compiler don't care).

template 
class Observer
{
    public:
        Observer()
            : obsID_(obsIDTracker_++)
        {}

        template 
        void OnNotifyImpl(T &subject, EventType event)
        {
            static_cast(this)->OnNotify(subject, event);
        }

        // I would still do this all in one line.
        int GetID() const { return obsID_; }

    private:
        int obsID_;
        static int obsIDTracker_;
};


This line confused me for a while:

Subject(std::tuple &&obs)                : observers(obs) {}
                        //   ^^  rvalue reference              ^^^ no move


You are giving the option to pass by r-value reference this implying you are going to rip the guts out of the object. But you are actually just making a copy into observers. A cheap copy as the tuple only contains references. But I think the interface is better if you pass by const reference to more accurately reflect the usage semantics better.

Subject(std::tuple const& obs)                : observers(obs) {}


OK found why you have r-value references:

template  class T, typename... Args, typename... Obs>
T MakeSubject(std::tuple &&obs, Args &&... args) {
  return T(std::move(obs), args...);
}


For the Obs is is unnecessary; as they are held in the tuple by reference. So std::move() here does not buy you much (apart from me scratching my head for 10 minutes :-)

On the other hand Args it does seem correct to pass by r-value reference, but because you don't use std::move they are decaying into references when you pass them to the constructor (I could be wrong on this last point (still getting my head around this new fangled stuff).


While the observers will typically be statically stored, there is nothing preventing a programmer from creating an observer in automatic storage then storing a reference to it in a subject that is retained past its destruction. I don't know if there's an easy way to prevent this, but it's a potential issue.

If there is no new usage I can not contrive of a situation where this would happen. As the Oberver must be constructed before the Subject because you add add Observers during the Subject construction.

Also I would argue that typical usage should be everything automatic not static.


In the Subject super class, I needed to do a cast: static_cast(this) as opposed to casting this to type T (where T is the subclass type.) This allows the Observer to overload based on T, but I'm worried it might not be safe in some scenarios. Is this valid?

This looks fine.


I dislike the verbosity required for creating a Subject subclass. Needing to type out the parameter pack so many times seems like it could be hard to work with, but with a good example maybe its okay?

You have done the hard work. So that the users of your lib don't need to worry about it. So I think you have hidden the details rather well. Its still verbose but with some good examples and an explanation you will be fine.


The MakeSubject function seems a little verbose too. Same as above -- is it an okay style if it's simply well-documented?

Same comment as above.

Code Snippets

template <typename Base> class Observer {

// I split this across two lines:
template <typename Base>
class Observer {
class Observer {
public:
  Observer() : obsID_(obsIDTracker_++) {}
  template <typename T> void OnNotifyImpl(T &subject, EventType event) {
    static_cast<Base *>(this)->OnNotify(subject, event);
  }
  int GetID() const { return obsID_; }
private:

// Everything is so squashed to the left:
// Bit more space to help readability never heart (and the compiler don't care).

template <typename Base>
class Observer
{
    public:
        Observer()
            : obsID_(obsIDTracker_++)
        {}

        template <typename T>
        void OnNotifyImpl(T &subject, EventType event)
        {
            static_cast<Base *>(this)->OnNotify(subject, event);
        }

        // I would still do this all in one line.
        int GetID() const { return obsID_; }

    private:
        int obsID_;
        static int obsIDTracker_;
};
Subject(std::tuple<Obs &...> &&obs)                : observers(obs) {}
                        //   ^^  rvalue reference              ^^^ no move
Subject(std::tuple<Obs &...> const& obs)                : observers(obs) {}
template <template <typename...> class T, typename... Args, typename... Obs>
T<Obs...> MakeSubject(std::tuple<Obs &...> &&obs, Args &&... args) {
  return T<Obs...>(std::move(obs), args...);
}

Context

StackExchange Code Review Q#38980, answer score: 3

Revisions (0)

No revisions yet.