patterncppMinor
Car with multiple drivetrain strategies
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:
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
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.oThe 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:
Have a generic strategy setter:
(Note: Ignore
Then you can set any strategy (even a completely new one that is invented later).
Code Review
Don't use prefix underscore on your identifiers. This is reserved for the implementation.
Thouse (_CCART_H_) are technically illegal identifiers.
Only include header files that you actually need.
These are not needed in CCar.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:
If you don't do the above you can easily break your code.
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,
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
So change
into
Now you can get rid of:
As these are handled for you by the
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.