patterncppMinor
Decoding and Encoding packet layers using decorator pattern
Viewed 0 times
decoratorpatternusinglayersdecodingandencodingpacket
Problem
I am trying to use decorator pattern to decode and encode packet layers in my application. I've supposed my packet has three layers: Header, Level2, Level3. So far I've come up with the following code. For the sake of simplicity, I've supposed that the data of each layer is the first 10 bytes of the input packet. I've also intentionally ignored memory leak problems.
Decoder.h file:
Decoder.cpp file:
```
#include "Decoder.h"
Header::Header(const std::string &data)
{
mData = data;
}
Header::~Header(){}
void Header::Decode(std::string &packet)
{
mData = packet.substr(0, 10);
mNextData = packet.substr(10);
#include
#include "Decoder.h"
int main()
{
Header* header = new Header("HeaderData");
Level2* l2 = new Level2(header, "Level2Data");
Level3* l3 = new Level3(l2, "Level3Data");
std::cout Encode() Encode() Encode() Encode();
header->Decode(packet);
l2->Decode(header->mNextData);
l3->Decode(l2->mNextData);
std::cout mData mData mData << std::endl;
}Decoder.h file:
#ifndef _DECODER_H_
#define _DECODER_H_
#include
class DecoderBase
{
public:
virtual void Decode(std::string &packet) = 0;
virtual std::string Encode() = 0;
protected:
DecoderBase* mInnerLayer;
};
class Header : public DecoderBase
{
public:
Header(const std::string &data);
virtual ~Header();
virtual void Decode(std::string &packet);
virtual std::string Encode();
std::string mData;
std::string mNextData;
};
class Level2 : public DecoderBase
{
public:
Level2(DecoderBase* innerLayer, const std::string &data);
~Level2();
virtual void Decode(std::string &packet);
virtual std::string Encode();
std::string mData;
std::string mNextData;
};
class Level3 : public DecoderBase
{
public:
Level3(DecoderBase* innerLayer, const std::string &data);
~Level3();
virtual void Decode(std::string &packet);
virtual std::string Encode();
std::string mData;
std::string mNextData;
};
#endifDecoder.cpp file:
```
#include "Decoder.h"
Header::Header(const std::string &data)
{
mData = data;
}
Header::~Header(){}
void Header::Decode(std::string &packet)
{
mData = packet.substr(0, 10);
mNextData = packet.substr(10);
Solution
I don't know about the decorator pattern, but there are things you can improve anyway:
-
Identifiers that begin with an underscore followed by a capital letter are reserved identifiers in C++. Therefore,
-
You never modify the string that you pass to
-
All of your class members are
-
You don't seem to need pointers in
-
-
Try to use constructor initialization lists whenever possible:
-
There are several functions that could be
-
Identifiers that begin with an underscore followed by a capital letter are reserved identifiers in C++. Therefore,
_DECODER_H_ is not a valid identifier; that name is reserved to the implementation. You should change it to DECODER_H_.-
You never modify the string that you pass to
Decode. Therefore, you should pass it a const reference instead of a simple non-const one:virtual void Decode(const std::string &packet) = 0;-
All of your class members are
public. Therefore, you could simply change them to structs instead and remove all the public qualifications. The only difference between class and struct is that all is public by default (even inheritance) while all is private by default within a class.-
You don't seem to need pointers in
main, regular automatic variables would be enough. Actually, try to use pointers only when you have to in C++, only where it makes sense. Otherwise, try to always use value and/or reference semantics. That way, you may avoid many problems.-
DecoderBase is probably meant to be a base class to be used for some polymorphic behaviour. Give it a virtual destructor. Otherwise, deleting a derived instance through a base pointer will be undefined behaviour.-
Try to use constructor initialization lists whenever possible:
Level2::Level2(DecoderBase *innerLayer, const std::string &data):
mInnerLayer(InnerLayer),
mData(data)
{}-
There are several functions that could be
const-qualified (try to do so whenever a function does not modify the members of the class). The Encode methods look like they should be const-qualified everywhere:std::string Header::Encode() const
{
return mData;
}Code Snippets
virtual void Decode(const std::string &packet) = 0;Level2::Level2(DecoderBase *innerLayer, const std::string &data):
mInnerLayer(InnerLayer),
mData(data)
{}std::string Header::Encode() const
{
return mData;
}Context
StackExchange Code Review Q#58816, answer score: 6
Revisions (0)
No revisions yet.