5
\$\begingroup\$

Please review my code below. I am studying the Strategy Design Pattern and I think I have implemented it correctly. Please check. I have tried to split the method definitions and the prototypes as much as possible so there are many files. Any comments are welcome.

The code is copied here by a one liner in from the Linux shell, so there are no copy-paste errors. I have no makefile, I used Code::Blocks; it generated the following g++ commands:

g++ -Wall -g  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/StrategyPattern/CCar.cpp -o obj/Debug/CCar.o
g++ -Wall -g  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/StrategyPattern/CFourWheelPowered.cpp -o obj/Debug/CFourWheelPowered.o
g++ -Wall -g  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/StrategyPattern/CFrontWheelPowered.cpp -o obj/Debug/CFrontWheelPowered.o
g++ -Wall -g  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/StrategyPattern/CPropagationBase.cpp -o obj/Debug/CPropagationBase.o
g++ -Wall -g  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/StrategyPattern/CRearWheelPowered.cpp -o obj/Debug/CRearWheelPowered.o
g++ -Wall -g  -c /mnt/home/Data_MaOt/Short_C_and_Cpp_progs/DesignPatterns/StrategyPattern/main.cpp -o obj/Debug/main.o
g++  -o bin/Debug/StrategyPattern obj/Debug/CCar.o obj/Debug/CFourWheelPowered.o obj/Debug/CFrontWheelPowered.o obj/Debug/CPropagationBase.o obj/Debug/CRearWheelPowered.o obj/Debug/main.o

The idea is that the CCar class has one way to propagate at a time, presented to it through a pointer iPropagationBasePtr that can contain objects of three derived classes: CFourWheelPowered, CFrontWheelPowered or CRearWheelPowered.

CCar.cpp

#include "CCar.h"

CCar::CCar()
{
    iPropagationBasePtr = new CPropagationBase();
}

CCar::~CCar()
{
    DeletePropagationMethod();
}

void CCar::DeletePropagationMethod()
{
    delete iPropagationBasePtr;
    iPropagationBasePtr = NULL;
}

void CCar::SetFrontWheelPowered()
{
    DeletePropagationMethod();
    iPropagationBasePtr = new CFrontWheelPowered();
}

void CCar::SetRearWheelPowered()
{
    DeletePropagationMethod();
    iPropagationBasePtr = new CRearWheelPowered();
}

void CCar::SetFourWheelPowered()
{
    DeletePropagationMethod();
    iPropagationBasePtr = new CFourWheelPowered();
}

void CCar::PropagateMyCar()
{
    if (iPropagationBasePtr != NULL)
    {
        iPropagationBasePtr->Propagate();
    }
    else
    {
        printf("CCar::PropagateMyCar(): Don't know how to propagate the car (iPropagationBasePtr= %p).\n", iPropagationBasePtr);
    }
}

CCar.h

#ifndef _CCAR_H_
#define _CCAR_H_

#include "CPropagationBase.h"

#include "CFourWheelPowered.h"
#include "CRearWheelPowered.h"
#include "CFrontWheelPowered.h"

class CCar
{
public:
    CCar();
    ~CCar();

    void SetFrontWheelPowered();
    void SetRearWheelPowered();
    void SetFourWheelPowered();
    void PropagateMyCar();

private:
    void DeletePropagationMethod();

    CPropagationBase* iPropagationBasePtr;


};

#endif // _CCAR_H_

CFourWheelPowered.cpp

#include "CFourWheelPowered.h"

void CFourWheelPowered::Propagate()
{
  printf("CFourWheelPowered::Propagate(): Propagating on four wheels.\n");
};

CFourWheelPowered::~CFourWheelPowered()
{
  printf("CFourWheelPowered::~CFourWheelPowered(): Started.\n");
}

CFourWheelPowered::CFourWheelPowered()
{
  printf("CFourWheelPowered::CFourWheelPowered(): Started.\n");
}

CFourWheelPowered.h

#ifndef _CFOURWHEELPOWERED_H_
#define _CFOURWHEELPOWERED_H_

#include "CPropagationBase.h"

class CFourWheelPowered : public CPropagationBase
{
public:
    void Propagate();
    CFourWheelPowered();
    ~CFourWheelPowered();
};


#endif // _CFOURWHEELPOWERED_H_

CFrontWheelPowered.cpp

#include "CFrontWheelPowered.h"

void CFrontWheelPowered::Propagate()
{
  printf("CFrontWheelPowered::Propagate(): Propagating on the front wheels.\n");
};

CFrontWheelPowered::~CFrontWheelPowered()
{
  printf("CFrontWheelPowered::~CFrontWheelPowered(): Started.\n");
}

CFrontWheelPowered::CFrontWheelPowered()
{
  printf("CFrontWheelPowered::CFrontWheelPowered(): Started.\n");
}

CFrontWheelPowered.h

#ifndef _CFRONTWHEELPOWERED_H_
#define _CFRONTWHEELPOWERED_H_

#include "CPropagationBase.h"

class CFrontWheelPowered : public CPropagationBase
{
public:
    void Propagate();
    CFrontWheelPowered();
    ~CFrontWheelPowered();
};


#endif // _CFRONTWHEELPOWERED_H_

CPropagationBase.cpp

#include "CPropagationBase.h"

void CPropagationBase::Propagate()
{
    printf("CPropagationBase::Propagate(): No idea how to propagate.\n");
}

CPropagationBase::~CPropagationBase()
{
    printf("CPropagationBase::~CPropagationBase(): Starting.\n");
}

CPropagationBase::CPropagationBase()
{
    printf("CPropagationBase::CPropagationBase(): Starting.\n");
}

CPropagationBase.h

#ifndef _CPROPAGATIONBASE_H_
#define _CPROPAGATIONBASE_H_

#include <stdio.h>

class CPropagationBase
{
public:
    virtual void Propagate();
    virtual ~CPropagationBase();
    CPropagationBase();
};

#endif // _CPROPAGATIONBASE_H_

CRearWheelPowered.cpp

#include "CRearWheelPowered.h"

void CRearWheelPowered::Propagate()
{
  printf("CRearWheelPowered::Propagate(): Propagating on the rear wheels.\n");
};

CRearWheelPowered::~CRearWheelPowered()
{
  printf("CRearWheelPowered::~CRearWheelPowered(): Started.\n");
}

CRearWheelPowered::CRearWheelPowered()
{
  printf("CRearWheelPowered::CRearWheelPowered(): Started.\n");
}

CRearWheelPowered.h

#ifndef _CREARWHEELPOWERED_H_
#define _CREARWHEELPOWERED_H_

#include "CPropagationBase.h"

class CRearWheelPowered : public CPropagationBase
{
public:
    void Propagate();
    CRearWheelPowered();
    ~CRearWheelPowered();
};


#endif // _CREARWHEELPOWERED_H_

main.cpp

#include <stdio.h>
#include "CCar.h"

int main()
{
    printf("main(): Starting.\n");

    CCar myCar;
    myCar.PropagateMyCar();

    myCar.SetFourWheelPowered();
    myCar.PropagateMyCar();

    myCar.SetFrontWheelPowered();
    myCar.PropagateMyCar();

    myCar.SetRearWheelPowered();
    myCar.PropagateMyCar();

    printf("main(): Finished.\n");
    return 0;
}


// Above: main.cpp
\$\endgroup\$
  • \$\begingroup\$ That's one way to do it, but there are many. Whether you choose an object method like this, plain function pointers, functors, etc. would depend on what data needs to be accessed and where that is stored. In this case, there is no data except for the pointer, so there's no discernible advantage to using this particular technique and some small disadvantages (e.g. the use of virtual functions). \$\endgroup\$ – Edward Feb 23 '17 at 17:34
  • \$\begingroup\$ Note: The disadvantage of virtual function mention by Edward is practically unmeasurable. Virtual tables tend to end up in the cache, so the single extra lookup is a register load from a level 1 cache. This is significantly outweighed by the advantages of using objects because of their flexibility (ability to cary data if needed). This extra flexibility makes long term maintenance and upgrades easier. So choose this object method first move to function pointers only if you actually need the extra speed and can prove you get a speed enhancement from the pointer technique. \$\endgroup\$ – Martin York Feb 24 '17 at 7:32
  • 1
    \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Feb 27 '17 at 10:16
  • 1
    \$\begingroup\$ @LokiAstari: "practically unmeasurable" and "level 1 cache" assumes that everything is an x86 desktop. Some of us do embedded systems which have no cache and for which virtual function call overhead is absolutely measurable. That's not to disagree that virtual functions are extremely useful (they are!), but just to underline that the usage context influences a lot of dimensions to this. As always, measuring the speed against the requirements, ideally on the real hardware and with the real data, can provide helpful input when making implementation decisions. \$\endgroup\$ – Edward Feb 27 '17 at 19:41
  • \$\begingroup\$ @Edward: Yes I make the assumption that everybody is always on big large servers all the time. I should not do that. \$\endgroup\$ – Martin York Feb 27 '17 at 21:03
7
\$\begingroup\$

Design

The implementation of the strategy looks fine. Though you limit yourselves to know strategies now. Rather than having the car know about the strategies:

    void SetFrontWheelPowered();
    void SetRearWheelPowered();
    void SetFourWheelPowered();

Have a generic strategy setter:
(Note: Ignore std::unique_ptr; I'll cover that below.)

    void setWheelPowerStrategy(std::unique_ptr<CPropagationBase>&&);

Then you can set any strategy (even a completely new one that is invented later).

    car.SetWheelPowerStrategy(make_unique<HoverConversionPower>());

Code Review

Don't use prefix underscore on your identifiers. This is reserved for the implementation.

#ifndef _CCAR_H_
#define _CCAR_H_

Thouse (_CCART_H_) are technically illegal identifiers.

Only include header files that you actually need.

These are not needed in CCar.h

#include "CFourWheelPowered.h"
#include "CRearWheelPowered.h"
#include "CFrontWheelPowered.h"

They may be needed in CCar.cpp (and they should be included there). But they are not needed in the header file.

Your class does not implement the rule of three (or five). Basically if you don't define the copy constructor or assignment operator then the compiler will generate one for you. In normal situations this is fine. But your class owns a pointer. This is the one scenario where the compiler generated versions of this will not work. So you should have implemented these two methods:

 CCar(CCar const& copy);
 CCar& operator=(CCar const& copy);

If you don't do the above you can easily break your code.

 {
     CCar  car1;        // normal car.
     CCar  car2(car1);  // car 2 is a copy of car1 and the 
                        // iPropagationBasePtr points at the same object.

 } // When the object go out of scope the destructor
   // will be called for each object. Thus calling delete on the
   // same pointer twice. Thus generating nasal lizards.

BUT there is another rule. The rule of zero. You can get around the rule of three if you implement the rule of zero. Which basically says that you should delegate ownership of resources to a class that is specialized for resource management. In C++ we have such a class, std::unique_ptr.

It is exceedingly rare in modern C++ to see pointer objects. Dynamically allocated memory is managed (usually) by smart pointers that guarantee that the memory is deleted (it is a better form of garbage collection). The std::unique_ptr is one of the standard smart pointers.

So change

    CPropagationBase* iPropagationBasePtr;

into

    std::unique_ptr<CPropagationBase> iPropagationBasePtr;

Now you can get rid of:

~CCar();
void DeletePropagationMethod();

As these are handled for you by the std::unique_ptr.

\$\endgroup\$
  • \$\begingroup\$ One could also use plain old C-style function pointers if, as is the case with this "toy" code, the called function needs no access to data stored in the base object. \$\endgroup\$ – Edward Feb 23 '17 at 17:28
  • \$\begingroup\$ @Edward: True; but I see no advantage to that (less the virtual call is a peco more expensive to call). The disadvantage is that it is less expandable in the future. \$\endgroup\$ – Martin York Feb 23 '17 at 17:33
  • \$\begingroup\$ The only advantages are simplicity and zero run-time overhead. Which is "better" would depend entirely on the (real) application and its associated requirements. \$\endgroup\$ – Edward Feb 23 '17 at 17:39
  • \$\begingroup\$ Thanks for your help guys. @Loki: I had never heard about this rule of three/five/zero. Thanks for bringing it up. I have googled on it too. I am going to use the smart pointers from now on; it is a C++11 feature. Thanks again. \$\endgroup\$ – TradingDerivatives.eu Feb 24 '17 at 14:48
  • \$\begingroup\$ @TradingDerivatives.eu The rule of three/five/zero \$\endgroup\$ – Martin York Feb 24 '17 at 17:22

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.