C++ Observer Pattern with std::function

Question

I am/we are using the observer pattern for many things in our application. I think it's time to create a reusable base class for these situations. Most implementations I could find only do something like this:

class Subject
{
public:
    void AttachObserver(Observer* observer)
    {
        observers.push_back(observer);
    }
    ...
};

I would prefer to be able to attach a callback function instead of a class. I came up with this:

namespace Observe
{
    using ObserverID = uint64_t;

    template<typename... callback_arg_types>
    class Obserable
    {
    public:
        using CallbackFunction = std::function<void(callback_arg_types...)>;

    public:
        ObserverID AttachObserver(CallbackFunction callback)
        {
            std::unique_lock<std::mutex>    block(_mutex);
            const auto id = GetNewID();
            _observers.emplace(id, std::move(callback));
            return(id);
        }

        bool DetachObserver(ObserverID& id)
        {
            std::unique_lock<std::mutex>    block(_mutex);
            const auto it = _observers.find(id);
            if (it == _observers.end())
            {
                return(false);
            }
            _observers.erase(it);
            id = ObserverID();
            return(true);
        }

    protected:
        void NotifyAllObservers(callback_arg_types... args)
        {
            std::unique_lock<std::mutex>    block(_mutex);
            for (const auto& observer : _observers)
            {
                observer.second(args...);
            }
        }


    private:
        ObserverID GetNewID()
        {
            static ObserverID id = 0;
            return(++id);
        }

        std::mutex                                          _mutex;
        std::unordered_map<ObserverID, CallbackFunction>    _observers;
    };
}

Usage:

using namespace Observe;

class IntPublisher : public Obserable<int> 
{
public:
    void Publish(int number) 
    {
        NotifyAllObservers(number);
    }
};

IntPublisher publisher;
ObserverID id = publisher.AttachObserver([](int number)->void{
    std::cout << number << std::endl;
});

publisher.Publish(42);
publisher.DetachObserver(id);

I don't like the ObserverID. I know I will never run out of ids, but I think there must be a more elegant solution to get a reference/handle to the attached observer/callback function (so that it can be detached later).

I would appreciate comments, corrections, criticism and suggestions.

Answer 1

I have mainly worked on the ObserverID part. I agree with you on one point, it is rather artificial, when we know that C++ functions are hashable, so you could use the hash as ID. A point we must be aware is that std::function objects are not hashable (do not ask me why...) or at least I cannot hash them but others could not either

For that reason, I would advise you to use plain function pointers instead of wrapping them into std::function objects: they add no functionality, at least no that you currently use, nor any I can imagine, and prevent hashing.

Additionaly, plain function pointers avoid the artificial by value parameter in AttachObserver. It was required because you wanted to be able to pass a lambda or plain function, and they have to be converted to a std::function. But as soon as you get a function pointer everything gets smooth. And it does not change the way the methods are used.

It has an additional feature: it prevents adding the same observer function more than once. If you really wanted that, you would just have to wrap the additional calls in lambdas.

You also have a useless id = ObserverID(); in DetachObserver, that you should remove.

Here is the changed code:

namespace Observe
{
    using ObserverID = std::size_t;    // the result of hashing

    template<typename... callback_arg_types>
    class Obserable
    {
    public:
        using CallbackFunction = void(*)(callback_arg_types...); // plain function pointer

    public:
        ObserverID AttachObserver(CallbackFunction callback)
        {
            std::unique_lock<std::mutex>    block(_mutex);
            ObserverID id = getID(callback);
            _observers.emplace(id, callback);
            return id;
        }

    ...

    private:

        ObserverID getID(void (*callback)(callback_arg_types...)) {
            typedef void (*func_type) (callback_arg_types...);
            static std::hash<func_type> hash;
            const ObserverID id = hash(callback);
            return id;
        }
        std::mutex                                          _mutex;
        std::unordered_map<ObserverID, CallbackFunction>    _observers;
    };
}

Answer 2

A few minor things:

return(false); etc.
Don't put extra parens around the return value. return is not a function call. It is good that it looks different from a function call since it stands out better as a different kind of thing. When you start using templates and implicit types (like auto) and automatic move (rather than copy) of local variables, it does change the meaning! Get out of that habit.

⧺SL.io.50 Don't use endl.

std::unique_lock<std::mutex> block(_mutex);
You can rely on class template argument deduction and not specify the type here. This makes the code easier to maintain, as when you change the exact type of _mutex then you don't have to go change every single usage as well!