Creating composite objects using the builder pattern

I need to decode and encode TLV (Tag, Length, Value) data which is a composite part-whole tree structure.

Tag1 Len1 Tag2-Len2-Value2 Tag3-Len3-Value3 ... TagN-LenN-ValueN
------------------------Value1------------------------


Therefore, I've decided to use the Composite pattern for the TLV objects:

typedef std::unique_ptr TLVComponentPtr;

class TLVComponent
{
public:
TLVComponent();
virtual ~TLVComponent();
virtual void Decode(const std::string &packet) = 0;
virtual std::string Encode() = 0;
virtual void AddTLV(TLVComponentPtr tlv); // only used in the constructor TLV
unsigned char GetTag();
unsigned char GetLen();
std::vector GetBytes();
std::string GetBytesStr();
protected:
unsigned char mTag;
unsigned char mLen;
std::vector mBytes;
};

class PrimitiveTLV : public TLVComponent
{
public:
PrimitiveTLV();
PrimitiveTLV(const std::string &packet);
PrimitiveTLV(int tag, const std::string &bytes);
virtual ~PrimitiveTLV();
virtual void Decode(const std::string &packet);
virtual std::string Encode();
virtual void AddTLV(TLVComponentPtr tlv); // throw exception if called
};

class ConstructorTLV : public TLVComponent
{
public:
ConstructorTLV(unsigned char tag);
ConstructorTLV(std::string packet);
virtual ~ConstructorTLV();
virtual void Decode(const std::string &packet);
virtual std::string Encode();
virtual void AddTLV(TLVComponentPtr tlv);
private:
std::vector mTLVs;
};


And here is the way I create a sample composite TLV object (please note that the real-world TLVs are not this simple and their values could be parameterized):

`std::string bytes = "";
bytes += 0x12;
bytes += 0x34;
unsigned char tag = 0x10;
PrimitiveTLV pTLV1(tag, bytes);

bytes = "";
bytes += 0x56;
bytes += 0x78;
tag = 0x20;
PrimitiveTLV pTLV2(tag, bytes);

bytes = "";
bytes += 0x91;
bytes += 0xA1;
tag = 0x30;
PrimitiveTLV pTLV3(tag, bytes);

tag = 0x20;
ConstructorTLV cTLV1(tag);
c

Here are some observations that may help you improve your code.

Reconsider the pattern

It seems to me that the Builder pattern is overkill for this application. Simply put, you have a TLV that might be either simple or composite (which are your PrimitiveTLV and ConstructorTLV classes). Since it seems that your primary interest is in the Encode() and Decode() methods, let's step back and ponder what we would like to be able to write without yet worrying about the mechanics of how this might be done. Your own proposed code looks like this:

TLVMaker tlvMaker;    
TLVComponentPtr tlvE = tlvMaker.MakeTLVE();
std::string tlvE_Encoded = tlvE->Encode();

This is peculiar to me in a few regards. First, if there are only five types of TLV, none of which have any supplied parameters, it would make more sense to me to simply have the five types statically created as constexpr and use them where needed with no runtime overhead at all.

However, it seems unlikely to me that this is really what you want. Instead, it seems more likely that you want a flexible means of making either simple or composite TLVs and that regardless of whether they're composite or simple, you'd like to have both Decode and Encode methods.

Don't use std::string for arbitrary byte strings

Your MakeTLVA() method includes these lines:

std::string bytes = "";
bytes += 0x91;

However, this has some problems. First, a std::string is just an alias for std::basic_string and a char can be either signed or unsigned -- it's implementation defined. The result is that bytes += 0x91; treats the number on the right as a signed int with the value of 145, and tries to put it into a char which, if it is signed, may only have a range of -128 to +127. You probably don't actually need or want signed values, so I would recommend that you use std::vector instead of std::string here.

Consider appropriate sizes

The TLVComponent has a member mLen which is defined as an unsigned char, but that would limit even a composite TLV to a maximum length of 255 bytes. In many such applications, that's too short and it would be better to be declared as unsigned, but perhaps your application only needs small TLVs.

Make const methods const

The GetTag() method probably just returns the tag value and doesn't alter the underlying object. Because it's both short and const I'd recommend putting it into the header file that way:

unsigned char GetTag() const { return mTag; }

The same is true for GetLen and probably some other methods.

Use consistent naming schemes for objects and methods

I generally prefer to have method names begin with lowercase letters and class names begin with uppercase letters. You are, of course, free to use a different scheme as long as you apply it consistently. It appears you have adopted the convention of using Uppercase letters for both class names and member functions.

Add member functions only to appropriate derived classes

The AddTLV() method is apparently supposed to throw an exception if it is called on a PrimitiveTLV. If that's the case, why put it in the interface at all? Instead, omit it from there and from the base class and only add it to the composite TLV class.

Omit the length because it's implicit

Since the underlying representation is at least partly a std::vector, you don't need to additionally store a length. Instead, simply calculate it as the length of the vector plus 2 (= 1 for mTag and 1 for mLen).

Create an initializer_list constructor

Here's what I'd rather write instead of your MakeTLVA() code:

const PrimitiveTLV a{0x10, {0x12, 0x34}};

That can easily be accomodated in C++11 using a constructor that includes std::initializer_list. Specifically, you could use this:

class PrimitiveTLV 
{
public:
    PrimitiveTLV(unsigned char tag, std::initializer_list l) 
        : mTag(tag), mData(l)
        { }
std::size_t size() const { return mData.size() + 2; }
std::vector encode() const { 
    std::vector ret;
    ret.reserve(size());
    ret.push_back(mTag);
    ret.push_back(mData.size());
    ret.insert(ret.end(), mData.begin(), mData.end());
    return ret;
}

private:
    unsigned char mTag;
    std::vector mData;
};

Naturally, you'll need to #include for this constructor.

Create a constructor that accepts other TLVs

After you've created your ConstructorTLV class out of other TLVs, there is no way within the interface to access the enclosed TLVs. If that's acceptable, then all you really need is the PrimitiveTLV class posted above augmented with a constructor that takes a list of other TLVs. That's just a little more complex than the constructor above. Here is the other constructor:

```
PrimitiveTLV(unsigned char tag, std::initializer_list t)
: mTag(tag),
mData()
{
std::vector v;
for (auto &tlv : t) {
v = tlv.encode();
mData.insert(mData.end

StackExchange Code Review Q#86578, answer score: 8