Chain-of-Responsibility in C++11 using smart pointers

I have implemented the CoR pattern in c++11 using smart_ptr for association between chain of receivers. The client owns all the receiver objects(which can be created and destroyed at runtime) and the receiver object maintain weak_ptr for subsequent request handler.

```
// ChainOfResponsibility.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"

#include
#include
#include
#include
#include

// Event, sent from sender to many receivers. The first parameter decides who should handle.
using Event = std::tuple;

//Abstract class which manages the link to the subsequent classes.
class Handler {
public:
Handler(std::shared_ptr successor = nullptr) :m_successor(successor) {}
virtual void handle(Event e) = 0;
virtual ~Handler() {}
protected:
std::weak_ptr m_successor;
};

//clients need to implement handle function
class Agent : public Handler {
public:
Agent(std::shared_ptr successor = nullptr):Handler(successor) {}
void handle(Event e) override {
if (std::get(e) == 1) {
std::cout (e) ptr(m_successor);
ptr->handle(e);
}
catch(std::bad_weak_ptr) {
std::cout successor = nullptr) :Handler(successor) {}
void handle(Event e) override {
if (std::get(e) == 2) {
std::cout (e) ptr(m_successor);
ptr->handle(e);
}
catch (std::bad_weak_ptr) {
std::cout successor = nullptr) :Handler(successor) {}
void handle(Event e) override {
if (std::get(e) == 3) {
std::cout (e) ptr(m_successor);
ptr->handle(e);
}
catch (std::bad_weak_ptr) {
std::cout ();
auto supervisor = std::make_shared(boss);
auto agent = std::make_shared(supervisor);

//events
std::vector events;
events.push_back(std::make_tuple(1, "Technical support"));
events.push_back(std::make_tuple(2, "Billing query"))

First nit: boss and supervisor strike me as basically synonymous. So it's kind of confusing that you're using both of them as identifiers in your program. Are these identifiers supposed to be meaningful, or are they just placeholders like Alice or Bob?

Also a nit: If your destructor doesn't do anything, don't write it.

virtual  ~Boss() override {}

is just a waste of characters. (The definition of ~Handler() is important because you have to hang the virtual specifier on it; but the definition of ~Boss() is perfectly redundant.)

//receivers may be released at run time.
boss.reset();

I think I don't get it. If this line had instead read supervisor.reset(), wouldn't boss have become unreachable, as far as handling requests was concerned? (Because requests go first to agent; then to agent->m_successor if it's non-null; but supervisor is gone, so handling just stops there.)

This seems like a bug in your design.

void handle(Event e) override {
    // ...
            ptr->handle(e);

This function call makes a copy of Event e, which might be expensive since Event contains an arbitrarily long std::string. You could fix this in either of two ways: either by changing the function signature to

void handle(const Event& e)

(throughout the whole hierarchy, of course), or else by changing the function call to

ptr->handle(std::move(e));  // move, don't copy

I know you said "performance isn't a concern", but any time you're using exception handling for control flow you should really reconsider your design. Instead of throwing and catching bad_weak_ptr, I'd recommend

if (std::get(e) == 1) {
        std::cout (e) handle(e);
    } else {
        std::cout << "No successor handler" << std::endl;
    }

As for how your list of handlers should be defined: almost certainly you should just use a list of handlers (i.e. std::list — or std::list> if you really want to keep your original design's ability to have handlers drop out of the list at random).

I don't think that a Handler ought to have any idea of its parent; unless... do you think that the parent of a given handler is part of its "identity"? For example, in my example above, do you think it does actually make sense that if an Agent's Supervisor dies, the Agent would have no way of contacting his late Supervisor's Boss, so certain requests would go unhandled? If that really is a feature-not-a-bug, then okay.

Lastly, even if everything else about this design were good, it's unsettling how easy it would be for someone to write a Handler class that just dropped messages on the floor.

class Stockboy : public Handler {
public:
    Stockboy(std::shared_ptr successor = nullptr) :Handler(successor) {}
    void handle(Event e) override {
        if (std::get(e) == 7) {
            std::cout (e) << std::endl;
        }
    }
};

IMO, it would be better to take the responsibility for forwarding unhandled messages and move that responsibility right out of the Handler class. Give it to a Dispatcher class whose job is to find the right Handler and call it. The Dispatcher might hold a mapping of ints to handlers so that it could find the right recipient quickly; or it might just query each handler in turn until some handler reports success.

StackExchange Code Review Q#159659, answer score: 4