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

Decoding and Encoding packet layers using decorator pattern

Submitted by: @import:stackexchange-codereview··
0
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.

#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;
};

#endif


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

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, _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.