4
\$\begingroup\$

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 <memory>
#include <tuple>
#include <string>
#include <iostream>
#include <vector>

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

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

//clients need to implement handle function
class Agent : public Handler {
public:
    Agent(std::shared_ptr<Handler> successor = nullptr):Handler(successor) {}
    void handle(Event e) override {
        if (std::get<0>(e) == 1) {
            std::cout << "Agent handled the request: " + std::get<1>(e) << std::endl;
        }
        else {
            try {
                std::shared_ptr<Handler> ptr(m_successor);
                ptr->handle(e);
            }
            catch(std::bad_weak_ptr) {
                std::cout << "No successor handler" << std::endl;
            }
        }

    }

    virtual  ~Agent() override {}
};

class Supervisor : public Handler {
public:
    Supervisor(std::shared_ptr<Handler> successor = nullptr) :Handler(successor) {}
    void handle(Event e) override {
        if (std::get<0>(e) == 2) {
            std::cout << "Supervisor handled the request: " + std::get<1>(e) << std::endl;
        }
        else {
            try {
                std::shared_ptr<Handler> ptr(m_successor);
                ptr->handle(e);
            }
            catch (std::bad_weak_ptr) {
                std::cout << "No successor handler" << std::endl;
            }
        }

    }
    virtual  ~Supervisor() override {}
};

class Boss : public Handler {
public:
    Boss(std::shared_ptr<Handler> successor = nullptr) :Handler(successor) {}
    void handle(Event e) override {
        if (std::get<0>(e) == 3) {
            std::cout << "Boss handled the request: " + std::get<1>(e) << std::endl;
        }
        else {
            try {
                std::shared_ptr<Handler> ptr(m_successor);
                ptr->handle(e);
            }
            catch (std::bad_weak_ptr) {
                std::cout << "No successor handler" << std::endl;
            }
        }

    }
    virtual  ~Boss() override {}
};


int main() {

    //three receivers owned by the client
    auto boss = std::make_shared<Boss>();
    auto supervisor = std::make_shared<Supervisor>(boss);
    auto agent = std::make_shared<Agent>(supervisor);

    //events
    std::vector<Event> events;
    events.push_back(std::make_tuple(1, "Technical support"));
    events.push_back(std::make_tuple(2, "Billing query"));
    events.push_back(std::make_tuple(1, "Product information query"));
    events.push_back(std::make_tuple(5, "Police issue"));
    events.push_back(std::make_tuple(3, "Enterprise client request"));

    for (auto e : events) {
        agent->handle(e);
    }

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

    for (auto e : events) {
        agent->handle(e);
    }

    return 0;
}

Please review this and suggest me the improvemnt.

Assumption: Performance is not a primary concern.

Please suggest me an elegant implementation if you have seen somewhere else.

\$\endgroup\$
3
\$\begingroup\$

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<0>(e) == 1) {
        std::cout << "Agent handled the request: " + std::get<1>(e) << std::endl;
    } else if (auto ptr = m_successor.lock()) {
        ptr->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<Handler> — or std::list<std::weak_ptr<Handler>> 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<Handler> successor = nullptr) :Handler(successor) {}
    void handle(Event e) override {
        if (std::get<0>(e) == 7) {
            std::cout << "Stockboy handled the request: " + std::get<1>(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.

\$\endgroup\$
  • \$\begingroup\$ boss.reset(); was to show, the sender doesn't worry about the end receivers. The sender can send even if the appropriate receivers leave the system. Anyway, thanks a lot for your suggestion for improvement. I \$\endgroup\$ – asit_dhal Apr 3 '17 at 21:42
  • \$\begingroup\$ If I put the resposibility to another class called Dispatcher, then this guy will be the one to receive the message(sender be aware of it) and forwarding to the appropriate calss. So, the dispatcher has to be modified everytime, I need a new handler. So, for one additional handler, I will have three different places to modify(1st is the construction, 2nd is the switch case inside dispatcher and 3rd is the actual handler). Am i Correct ? \$\endgroup\$ – asit_dhal Apr 3 '17 at 21:45
  • \$\begingroup\$ @asit_dhal: Correct; the Dispatcher would need to know about all the Handlers in the system. That might involve a switch case inside Dispatcher; but it could equally well involve a dynamic map inside Dispatcher and a run-time function such as myDispatcher.register_handler(someNewHandler) that added handlers to that map. \$\endgroup\$ – Quuxplusone Apr 4 '17 at 1:46
  • \$\begingroup\$ Also, I didn't mention it in my answer, but if I were you, one of my goals would be to trim down class Handler until it was nothing more than a void handle(Event) function; and then rename that function to void operator()(Event) const; and then replace class Handler and all its subclasses with plain old lambdas. As in: myDispatcher.registerHandler(3, [](Event e){ std::cout << "Boss handled it!"; return true; }) \$\endgroup\$ – Quuxplusone Apr 4 '17 at 1:48

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.