0
\$\begingroup\$

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.

\$\endgroup\$
0
0
\$\begingroup\$

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;
    };
}
\$\endgroup\$
3
  • \$\begingroup\$ I don't usually like simple function pointers. But this is a really elegant way. Thanks a lot for that! As for id = ObserverID(), it's supposed to reset the ID. I use it as a kind of cleanup strategy so that the client is not left with junk. \$\endgroup\$
    – Michael
    Oct 22 at 7:22
  • \$\begingroup\$ Well, for std::function to be hashable, someone has to implement it, and its relationship to hashability of any kind of invokable. Then they must document it, and submit it for standardization. It would be an ABI-change if the hash-code of the invokable should be returned, as that must be retrieved, meaning one more callback. \$\endgroup\$ Oct 22 at 16:40
  • \$\begingroup\$ Primary reason for std::function not being hashable is that it is completely meaningless. It has no equality operator! What would you ever do with hashing if there is no way to compare them in the first place? And it is impossible to implement comparison operator without adding restrictions to the wrapping capabilities of the class. \$\endgroup\$
    – ALX23z
    Oct 24 at 4:17
0
\$\begingroup\$

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!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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