Decoding and Encoding packet layers using decorator pattern

Question

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 <iostream>
#include "Decoder.h"
int main()
{
 Header* header = new Header("HeaderData");
 Level2* l2 = new Level2(header, "Level2Data");
 Level3* l3 = new Level3(l2, "Level3Data");
 std::cout << header->Encode() << std::endl;
 std::cout << l2->Encode() << std::endl;
 std::cout << l3->Encode() << std::endl;
 std::string packet = l3->Encode();
 header->Decode(packet);
 l2->Decode(header->mNextData);
 l3->Decode(l2->mNextData);
 std::cout << ((Header*)header)->mData << std::endl;
 std::cout << ((Level2*)l2)->mData << std::endl;
 std::cout << ((Level3*)l3)->mData << std::endl;
}

Decoder.h file:

#ifndef _DECODER_H_
#define _DECODER_H_
#include <string>
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);
}
std::string Header::Encode()
{
 return mData;
}
Level2::Level2(DecoderBase *innerLayer, const std::string &data)
{
 mInnerLayer = innerLayer;
 mData = data;
}
void Level2::Decode(std::string &packet)
{
 mData = packet.substr(0, 10);
 mNextData = packet.substr(10);
}
std::string Level2::Encode()
{
 return mInnerLayer->Encode() + mData;
}
Level3::Level3(DecoderBase *innerLayer, const std::string &data)
{
 mInnerLayer = innerLayer;
 mData = data;
}
void Level3::Decode(std::string &packet)
{
 mData = packet.substr(0, 10);
 mNextData = packet.substr(10);
}
std::string Level3::Encode()
{
 return mInnerLayer->Encode() + mData;
}

Output:

HeaderData
HeaderDataLevel2Data
HeaderDataLevel2DataLevel3Data
HeaderData
Level2Data
Level3Data

1. Am I using the decorator pattern correctly to decode and encode packets?
2. Who is responsible for calling the Decode method of the next layer? Am I calling the Decode method of each layer correctly? I thought maybe l2->Decode() should be called from within header->Decode().

Any advice on improving the above code would be appreciated!

• jliv902: "I've also intentionally ignored memory leak problems." There is really no excuse to do that in C++. Even if you don't have access to smart pointers, you could still wrap your objects up in a RAII manner.

Answer 1

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

Answer 2

• This may just be something relating to font, but your variables l2 and l3 look much like the numbers 12 and 13 respectively. It's of course not an issue for the compiler, but a human may misinterpret it as numbers and wonder why you've tried to do that. You can just name them level2 and level3 respectively. It's okay if they match the type name closely, just as long as they still use different cases.

• You really don't need std::endl, unless you're also trying to flush the buffer. This will make your code a bit slower. Instead, just output "\n" within the stream to produce a newline.

• The function Encode() does not at all do what the name dictates. It only returns the value of a data member, making it an accessor. A better name for this could be getData(), or something that accurately describes this particular data. The get at the front makes it obvious that the function is fetching something.

Answer 3

Your decoration structure seems to be the wrong way round. From your description the header is the outermost layer, then comes layer 2 and then 3. Yet the way you construct your layers it is the other way around, this is especially obvious where you name the constructor parameter as innerLayer and pass something in which should be an outer layer.

So Layer3 should be the innermost layer:

Layer3 l3(null, data);
Layer3 l2(&l3, data);
Header h(&l2, data);

Same in your Encode implementations you do inner + this_layer - assuming that the beginning of the string should represent the outermost layer (the first thing visible) then this is the wrong way around as well. Similar for Decode.