2
\$\begingroup\$

I am learning design patterns from Design pattern for dummies. From that book I have written a chain of responsibility pattern with two programs. Can someone review and let me know which one is proper chain of responsibility design pattern?

A)

#include<iostream>

using std::cout;
using std::endl;

const int FRONT_END_HELP = 0;
const int INTERMEDIATE_LAYER_HELP = 1;
const int GENERAL_HELP = 2;

class HelpInterface
{
public:
    virtual void getHelp(int iHelpConstant) = 0;
};

class FrontEnd : public HelpInterface {
    HelpInterface * Successor;

public:
        FrontEnd(HelpInterface * s){ Successor = s;}
        void getHelp(int iHelpConstant);
};

void FrontEnd::getHelp(int iHelpConstant)
{
    if (FRONT_END_HELP == iHelpConstant)    {
        cout<<"This is the front end, don't you like it ?\n"<<endl;
    } else {
        Successor->getHelp(iHelpConstant);
    }
}

class IntermediateLayer : public HelpInterface{
      HelpInterface * Successor;
public:
    IntermediateLayer(HelpInterface * s) {Successor = s;}
    void getHelp(int iHelpConstant);
};

void IntermediateLayer::getHelp(int iHelpConstant)
{
    if (INTERMEDIATE_LAYER_HELP == iHelpConstant ) {
        cout<<"This is intermediate layer, nice eh?\n"<<endl;
    } else {
        Successor->getHelp(iHelpConstant);
    }
}

class Application : public HelpInterface{

public:
    Application() {}
    void getHelp(int iHelpConstant) {
        cout<<"This the application help\n"<<endl;
    }
};

int main()
{
    Application *app = new Application();
    IntermediateLayer *ilayer = new IntermediateLayer(app);
    FrontEnd * fend = new FrontEnd(ilayer);

    fend->getHelp(GENERAL_HELP);

    fend->getHelp(INTERMEDIATE_LAYER_HELP);

    system("Pause");

}

B)

#include<iostream>

using std::cout;
using std::endl;

const int FRONT_END_HELP = 0;
const int INTERMEDIATE_LAYER_HELP = 1;
const int GENERAL_HELP = 2;

class HelpInterface
{
protected :
    HelpInterface * Successor;
public:
    virtual void getHelp(int iHelpConstant) = 0;
};

class FrontEnd : public HelpInterface {

public:
        FrontEnd(HelpInterface * s){ Successor = s;}
        FrontEnd(){ Successor = NULL;}
        void getHelp(int iHelpConstant);
};

void FrontEnd::getHelp(int iHelpConstant)
{
    if (FRONT_END_HELP == iHelpConstant)    {
        cout<<"This is the front end, don't you like it ?\n"<<endl;
    } else {
        if (Successor != NULL){
            Successor->getHelp(iHelpConstant);
        } else {
            cout<<"No successor\n"<<endl;
        }
    }
}

class IntermediateLayer : public HelpInterface {
public:
    IntermediateLayer(HelpInterface * s) {Successor = s;}
    IntermediateLayer(){ Successor = NULL;}
    void getHelp(int iHelpConstant);
};

void IntermediateLayer::getHelp(int iHelpConstant)
{
    if (INTERMEDIATE_LAYER_HELP == iHelpConstant ) {
        cout<<"This is intermediate layer, nice eh?\n"<<endl;
    } else {
        if (Successor != NULL){
            Successor->getHelp(iHelpConstant);
        } else {
            cout<<"No successor\n"<<endl;
        }
    }
}

class Application : public HelpInterface{

public:
    Application() {Successor = NULL;}
    void getHelp(int iHelpConstant) {
        cout<<"This the application help\n"<<endl;
    }
};

int main()
{
    Application *app = new Application();
    IntermediateLayer *ilayer = new IntermediateLayer(app);
    FrontEnd * fend = new FrontEnd(ilayer);

    fend->getHelp(GENERAL_HELP);

    fend->getHelp(INTERMEDIATE_LAYER_HELP);

    system("Pause");
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Don't use NULL in C++ code. Use nullptr. \$\endgroup\$ – glampert Oct 22 '14 at 13:55
  • \$\begingroup\$ Please don't add new additional code for feedback to the same question after receiving answers. You may ask a new separate question instead. \$\endgroup\$ – Jamal Oct 26 '14 at 17:30
  • \$\begingroup\$ That code was not the for the feedback to the question but it was for reference for somebody like me learning it. Anyways if it is not as per policy i am okay. \$\endgroup\$ – AnkurTank Oct 26 '14 at 17:38
6
\$\begingroup\$

Design Pattern Comments

It works as a chain of responsibility (and the point of patterns is that they are not exact and are chained depending on exact need).

BUT saying that I think you could have done it better in this situation.

  1. Your descent of the the chain is manual and repeated in any every class.
    This should be automated in the base class. Each derived class should simply perform the action that you wan them too.
    This will remove programmer error when a new developer creates a chaining task.
  2. Your creation of the chain elements is not consistent. It would be easier to have a setTask() method then force every new task to join the chain rather than force every class to have a boiler plate constructor (that can be done incorrectly).
  3. You do not always want to chain all the way to the end.
    On of the handlers should be able to say I have done the final processing stop searching the chain.
  4. No memory management (but that was covered by @Mantosh Jumar).
    Also note using smart pointers helps you with another design pattern (Separation of Concerns).

I would have started like this:

class HelpInterface
{
        // Private virtual function.
        // You don't want it called directly as it just does the action
        // None of the chaining.
        virtual bool getHelp(int iHelpConstant) = 0;
        // Note it returns bool.
        // true indicates I have done it all.
        // Do not activate any more tasks in the chain.
        // false indicates try the next item in the chain.


        // Always have a next pointer in all HelpInterface
        std::unique_ptr<HelpInterface>   next;

    public:

        // Need  virtual destructor to make sure memory management is done
        // done correctly. As elements are destroyed via a pointer to the 
        // base class.
        virtual ~HelpInterface() {}

        // Chain the getHelp.
        // But stop if it returns true (or we are the last element
        // in the chain.
        void activateHelp(int iHelpConstant)
        {
            if (!getHelp(iHelpConstant) && next)
            {
                next->activateHelp(iHelpConstant);
            }
        }

        void setNextTask(std::unique_ptr<HelpInterface>&& nextElement)
        {
            next = std::move(nextElement);
        }

};

Now you can create the chain like this:

int main()
{
    std::unique_ptr<HelpInterface> app    = std::make_unique<Application>();
    std::unique_ptr<HelpInterface> ilayer = std::make_unique<IntermediateLayer>();
    std::unique_ptr<HelpInterface> fend   = std::make_unique<FrontEnd>();


    iLayer->setNextTask(std::move(app));
    fend->setNextTask(std::move(iLayer));


    fend->getHelp(GENERAL_HELP);
    fend->getHelp(INTERMEDIATE_LAYER_HELP);
}

General Coding comments

A tiny bit lazy here.

using std::cout;
using std::endl;

But these are used so often I will not complain. I would have preferred it if you had not polluted the global namespace for everybody (in this file).

All caps identifier are by tradition reserved for macros. Macros have no regard for scope so you are putting yourself in a dangerous position using all caps for constants.

const int FRONT_END_HELP = 0;
const int INTERMEDIATE_LAYER_HELP = 1;
const int GENERAL_HELP = 2;

Also could have used an enumerate. I personally would have done.

enum HelperTypes { FrontEndHelp, IntermediateLayerHelp, GeneralHelp};

Doing it this way, makes the compiler generate the actual numbers (so there is no chance that somebody accidentally puts two identical numbers into the list). Also if you change your getHelp() signature to use the enum people can not accidentally pass an integer, as the compiler will spot that and force them to use the correct type.

A base class with virtual members. Must usually have a virtual destructor.

class HelpInterface
{
protected :
    HelpInterface * Successor;
public:
    virtual void getHelp(int iHelpConstant) = 0;
};

Also in C++ never us RAW pointers (there are some exceptions). But RAW pointers do not convey ownership. If you don't know the ownership then you don't know who is responsible for deleting the object. One of the most important concepts in C++ (over C) is the concept of ownership (RAII being more important). We should always know who owns a pointer so we know who is going to clean it up. Smart pointers are the usuall technique of indicating ownership (or using references to indicate the passing of a reference with no ownership transfer).

The * is part of the type. So put the * by the type when declaring pointers.

 HelpInterface*     successor;   // object (anything you can get an address of)
                                 // this includes functions. Should have an
                                 // initial lower case letter.

 // User defined types have an initial uppercase letter.
 // This helps you distinguish Types from objects.

You use at least two different styles of bracing:

class HelpInterface
{                                       // Brace on the next line.

class FrontEnd : public HelpInterface { // Brace on the same line.

Pick one and be consistent.

Note: You can use different styles in different places.

class X
{                 // Style for classes.

    int func() {  // Style for code.

    }
};

Personally I always put the '{' on a new line by itself. Others find this wastes vertical space. I buy a big monitor and keep my functions relatively short.

Use default parameters to avoid this situation.

        FrontEnd(HelpInterface * s){ Successor = s;}
        FrontEnd(){ Successor = NULL;}

Can be replaced with:

        FrontEnd(HelpInterface* successor = nullptr)
            : successor(successor) // Note:  member and parameter name the same.
        {}

You have plenty of space between operators here:

    if (FRONT_END_HELP == iHelpConstant)    {

But here they are all packed together. Space is your frined use it.

        cout<<"This is the front end, don't you like it ?\n"<<endl;

Don't use std::endl unless you want to force a flush. This is rarely the case. std::cin and std::cout are linked. If user input is required the cout is automatically flushed so the user can see any questions (or output). Excessive flushing severly degrades performance.

The most hidious of things ever:

    system("Pause");

Stop using it.
Try.

    std::cout << "Hit Enter to continue\n";
    std::string line;
    std::getline(std::cin, line);

This is portable.

\$\endgroup\$
  • \$\begingroup\$ Somehow i am not able to use std::make_unique :( , which file i should include to use std::make_unique ? \$\endgroup\$ – AnkurTank Oct 26 '14 at 16:35
  • \$\begingroup\$ @AnkurTank: std::make_unique is C++14, defined in <memory>. \$\endgroup\$ – firda Oct 26 '14 at 17:38
  • \$\begingroup\$ @Firda I tried including it but it didn't work. It seems what Jamal says in another comment is true. May be my compiler is old and because of which i am not able to use/compile with std::make_unique. \$\endgroup\$ – AnkurTank Oct 26 '14 at 17:41
  • \$\begingroup\$ @AnkurTank: Don't worry about it then, use the constructor, as make_unique is equivalent to unique_ptr<T>(new T(std::forward<Args>(args)...)) ... or define your own make_unique (I am porting C++14 features to C++11 this way - to my own namespace). \$\endgroup\$ – firda Oct 26 '14 at 17:57
5
\$\begingroup\$

Both implementations are conceptually correct. Both are proper, with technical differences:

  • In the first, HelpInterface has no data, it only defines behavior. This is simple, and often much less tricky compared to when data is involved. Including data or not always has a trade-off: including data can reduce duplication in implementors, not including data gives greater flexibility to implementors.

  • In the second, all implementors provide a default constructor that will set Successor = NULL. This difference is mostly irrelevant: this is a convenience constructor, users can achieve the same by calling the single parameter constructor with a NULL parameter. You could have added this in the first version too, it's not a qualitative difference.

  • In the second, you check if Successor is NULL. This is safer, but tedious. You could have added this in the first version too.

I think all this boils down to the questions:

  1. Should HelpInterface contain data?
  2. Should you provide the parameterless convenience constructor?
  3. Should you check if Successor is NULL?
  4. How to minimize boilerplate, repetitive code?

My opinion:

  1. Should HelpInterface contain data?

It's nice and simple and clean if it doesn't. It can, if you really want. But it doesn't have to. Instead of adding data to this interface, you could create a sub-class that can contain data, for example:

class AbstractHelpInterface : public HelpInterface {
protected:
    HelpInterface * successor;
    AbstractHelpInterface() : successor(NULL) {}
    AbstractHelpInterface(HelpInterface * s) : successor(s) {}
};

Other classes can, if they want to, inherit from this instead of HelpInterface, for example:

class IntermediateLayer : public AbstractHelpInterface {
public:
    IntermediateLayer(HelpInterface * s) : AbstractHelpInterface(s) {}
    void getHelp(int iHelpConstant);
};

void IntermediateLayer::getHelp(int iHelpConstant) {
    if (INTERMEDIATE_LAYER_HELP == iHelpConstant ) {
        cout<<"This is intermediate layer, nice eh?\n"<<endl;
    } else if (successor != NULL) {
        successor->getHelp(iHelpConstant);
    }
}

Having both options HelpInterface and AbstractHelpInterface, implementors are free to choose whichever is most appropriate for their purposes.

  1. Should you provide the parameterless convenience constructor?

Yes. If such constructor doesn't exist, it would make me wonder how to deal with the last item in the chain. It would seem logical that I should use NULL as the param, but that still doesn't sound natural. If there is no next item, I shouldn't have to specify anything. I shouldn't have to make guesses and hope for the best that the code will work as intended. (An assumption that your first implementation would fail.)

  1. Should you check if Successor is NULL?

Yes. It is a possible value, so this cannot be avoided, otherwise the program will malfunction, which is not pretty.

  1. How to minimize boilerplate, repetitive code?

In addition to AbstractHelpInterface, you could add another kind of helper abstract class, and make use of the template method pattern to handle the helpConstant:

class ConstantMatchingHelpInterface : public HelpInterface {
private:
    int helpConstant;
    HelpInterface * successor;
protected:
    ConstantMatchingHelpInterface(int c) : helpConstant(c), successor(NULL) {}
    ConstantMatchingHelpInterface(int c, HelpInterface * s) : helpConstant(c), successor(s) {}
    virtual void getSpecificHelp() = 0;
public:
    void getHelp(int helpConstant);
};

void ConstantMatchingHelpInterface::getHelp(int helpConstant) {
    if (this->helpConstant == helpConstant) {
        getSpecificHelp();
    } else if (successor != NULL) {
        successor->getHelp(helpConstant);
    }
}

With the help of this, FrontEnd could become simpler:

class FrontEnd : public ConstantMatchingHelpInterface {
public:
    FrontEnd(HelpInterface * s) : ConstantMatchingHelpInterface(FRONT_END_HELP, s) {}
protected:
    void getSpecificHelp();
};

void FrontEnd::getSpecificHelp() {
    cout << "This is the front end, don't you like it ?\n" << endl;
}

Coding style

Use CamelCase for class names only, use lowerCamelCase for variables.


Note that you can simplify field initializations, from this:

FrontEnd(HelpInterface * s){ Successor = s;}
FrontEnd(){ Successor = NULL;}

Like this:

FrontEnd(HelpInterface * s = nullptr) : Successor(s) {}

This nested conditional can be simplified:

if (FRONT_END_HELP == iHelpConstant)    {
    cout<<"This is the front end, don't you like it ?\n"<<endl;
} else {
    if (Successor != NULL){
        Successor->getHelp(iHelpConstant);
    } else {
        cout<<"No successor\n"<<endl;
    }
}

Like this:

if (FRONT_END_HELP == iHelpConstant)    {
    cout<<"This is the front end, don't you like it ?\n"<<endl;
} else if (Successor != NULL){
    Successor->getHelp(iHelpConstant);
} else {
    cout<<"No successor\n"<<endl;
}

I recommend to use spaces more generously around operators, in particular, instead of this:

cout<<"No successor\n"<<endl;

It's more readable like this:

cout << "No successor\n" << endl;
\$\endgroup\$
4
\$\begingroup\$

The design pattern implementation seems to be correct. But I would like to point out few things from your implementation. You have not freed the objects created in your program. There is memory leak.

Application *app = new Application();
IntermediateLayer *ilayer = new IntermediateLayer(app);
FrontEnd * fend = new FrontEnd(ilayer);

So we can use the std::unique_ptr or std::shared_ptr.

std::unique_ptr<HelpInterface> app = std::make_unique<Application>();

As in above implementation everything is derived from the HelpInterface class, ideally we should create our objects of this type.

Programming to an Interface, not an Implementation

This way client would not know about exact type of class. So we should create our objects as below.

std::unique_ptr<HelpInterface> app    = std::make_unique<Application>();
std::unique_ptr<HelpInterface> ilayer = std::make_unique<IntermediateLayer>(app);
std::unique_ptr<HelpInterface> fend   = std::make_unique<FrontEnd>(ilayer);
\$\endgroup\$
  • \$\begingroup\$ Thank you for comments Mantosh. are std::unique_ptr & std::shared_ptr kind of smart pointers ? \$\endgroup\$ – AnkurTank Oct 22 '14 at 8:58
  • \$\begingroup\$ I searched it and found that online, indeed it is smart pointer, thank you. \$\endgroup\$ – AnkurTank Oct 22 '14 at 9:05
  • \$\begingroup\$ I am not able to use std::make_unique it gives error Error :namespace "std" has no member "make_unique" \$\endgroup\$ – AnkurTank Oct 26 '14 at 16:44
  • \$\begingroup\$ @AnkurTank: std::make_unique is actually introduced in C++14, so your compiler must not support that. \$\endgroup\$ – Jamal Oct 26 '14 at 17:33
  • \$\begingroup\$ @Jamal Will latest version of gcc support it ? \$\endgroup\$ – AnkurTank Oct 26 '14 at 17:40

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.