Creating composite objects using the builder pattern

Question

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<TLVComponent> 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<unsigned char> GetBytes();
    std::string GetBytesStr();
protected:
    unsigned char mTag;
    unsigned char mLen;
    std::vector<unsigned char> 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<TLVComponentPtr> 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);
cTLV1.AddTLV(&pTLV1);
cTLV1.AddTLV(&pTLV2);

tag = 0x80;
ConstructorTLV cTLV3(tag);
cTLV3.AddTLV(&cTLV1);
cTLV3.AddTLV(&pTLV3);

std::string cTLV3Encoded = cTLV3.Encode();

As the creation of the cTLV3 is complex, error-prone and is done via several steps, I have decided to use the Builder Pattern to create my TLV objects:

The problem is that I don't know how the Builder abstract and concrete classes fit in my design. I think the Director class of the pattern suffices for me:

///
/// \brief The TLV maker. (Director role in the Builder pattern)
///
class TLVMaker
{
public:
    TLVMaker();
    virtual ~TLVMaker();
    ///
    /// \brief Creates the primitive TLV A
    /// \return 
    ///
    TLVComponentPtr MakeTLVA();
    ///
    /// \brief Creates the primitive TLV B
    /// \return 
    ///
    TLVComponentPtr MakeTLVB();
    ///
    /// \brief Creates the primitive TLV C
    /// \return 
    ///
    TLVComponentPtr MakeTLVC();

    ///
    /// \brief Creates the constructor TLV D
    /// \return 
    ///
    TLVComponentPtr MakeTLVD();
    ///
    /// \brief Creates the constructor TLV E
    /// \return 
    ///
    TLVComponentPtr MakeTLVE();
};

And the implementation of the Director:

TLVComponentPtr TLVMaker::MakeTLVA()
{
    std::string bytes = "";
    bytes += 0x12;
    bytes += 0x34;
    uchar tag = 0x10;
    TLVComponentPtr tlv(new PrimitiveTLV(tag, bytes));
    return tlv;
}

TLVComponentPtr TLVMaker::MakeTLVB()
{
    std::string bytes = "";
    bytes += 0x56;
    bytes += 0x78;
    uchar tag = 0x20;
    TLVComponentPtr tlv(new PrimitiveTLV(tag, bytes));
    return tlv;
}

TLVComponentPtr TLVMaker::MakeTLVC()
{
    std::string bytes = "";
    bytes += 0x91;
    bytes += 0xA1;
    uchar tag = 0x30;
    TLVComponentPtr tlv(new PrimitiveTLV(tag, bytes));
    return tlv;
}

TLVComponentPtr TLVMaker::MakeTLVD()
{
    uchar tag = 0x20;
    TLVComponentPtr tlv(new ConstructorTLV(tag));
    tlv->AddTLV(MakeTLVA());
    tlv->AddTLV(MakeTLVB());
    return tlv;
}

TLVComponentPtr TLVMaker::MakeTLVE()
{
    uchar tag = 0x80;
    TLVComponentPtr tlv(new ConstructorTLV(tag));
    tlv->AddTLV(MakeTLVD());
    tlv->AddTLV(MakeTLVC());
    return tlv;
}

The creation of my sample TLV object becomes very simple now:

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

My questions:

1. Have I correctly decided to use the Builder pattern to create my composite objects?
2. I have only been able to use the Director class of the Builder pattern. How can/should I include the abstract and concrete Builder classes as well?
3. If using the Director class (TLVMaker) in my design is enough, can I still claim I have used the Builder pattern?

Answer 1

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<char> 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<unsigned char> 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<unsigned char> l) 
        : mTag(tag), mData(l)
        { }
std::size_t size() const { return mData.size() + 2; }
std::vector<unsigned char> encode() const { 
    std::vector<unsigned char> 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<unsigned char> mData;
};

Naturally, you'll need to #include <initializer_list> 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<PrimitiveTLV> t)
    : mTag(tag),
      mData()
    {
        std::vector<unsigned char> v;
        for (auto &tlv : t) {
           v = tlv.encode();
           mData.insert(mData.end(), v.begin(), v.end());
        }
    }

Try it with a test driver

It's always good to have at least a simple test program to try out a new class. Here's what I used:

TLV.h

#ifndef TLV_H
#define TLV_H
#include <initializer_list>
#include <numeric>
#include <vector>

class TLV 
{
public:
    TLV(unsigned char tag, std::initializer_list<unsigned char> l) 
        : mTag(tag), mData(l)
        { }
    TLV(unsigned char tag, std::initializer_list<TLV> t)
        : mTag(tag),
          mData()
          {
              std::vector<unsigned char> v;
              for (auto &tlv : t)
              {
                  v = tlv.encode();
                  mData.insert(mData.end(), v.begin(), v.end());
              }
          }
    // return size of encoded TLV (not just data size)
    std::size_t size() const { return mData.size() + 2; }
    std::vector<unsigned char> encode() const { 
        std::vector<unsigned char> 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<unsigned char> mData;
};
#endif // TLV_H

main.cpp

#include <iostream>
#include <iomanip>

#include "TLV.h"

using namespace std;

void dump(const TLV &a) {
    for (auto &ch : a.encode())
        cout << hex << setw(2) << setfill('0') 
             << static_cast<unsigned>(ch) << ' ';
    cout << endl;
}

int main()
{

    const TLV a{0x10, {0x12, 0x34}};
    const TLV b{0x20, {0x56, 0x78}};
    const TLV c{0x30, {0x91, 0xA1}};
    const TLV d{0x20, {a, b}};
    const TLV e{0x80, {d, c}};
    dump(a);
    dump(d);
    dump(c);
    dump(e);
}

Here was the result of running that program:

10 02 12 34 
20 08 10 02 12 34 20 02 56 78 
30 02 91 a1 
80 0e 20 08 10 02 12 34 20 02 56 78 30 02 91 a1 

As you can see, all of the length calculations and creation of composite TLVs was handled very simply using only a single class. For that reason, I have actually renamed it simply TLV. In real production code, you would want more sophisticated handling for the length and the length encoding (for example, if the length is larger than would fit in a single byte), but it is not difficult to add this.