HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Car with multiple drivetrain strategies

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
carwithdrivetrainmultiplestrategies

Problem

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()
{
Delet

Solution

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&&);


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

car.SetWheelPowerStrategy(make_unique());


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 iPropagationBasePtr;


Now you can get rid of:

~CCar();
void DeletePropagationMethod();


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

Code Snippets

void SetFrontWheelPowered();
    void SetRearWheelPowered();
    void SetFourWheelPowered();
void setWheelPowerStrategy(std::unique_ptr<CPropagationBase>&&);
car.SetWheelPowerStrategy(make_unique<HoverConversionPower>());
#ifndef _CCAR_H_
#define _CCAR_H_
#include "CFourWheelPowered.h"
#include "CRearWheelPowered.h"
#include "CFrontWheelPowered.h"

Context

StackExchange Code Review Q#156105, answer score: 7

Revisions (0)

No revisions yet.