Polymorphic TLV serialization

Question

Inspired by this question I decided to write an alternative that uses polymorhphism and a Factory pattern. The code works for the subset of implemented types, namely BYTE, WORD and STRING but I have a few concerns:

1. I'd like to get rid of the big ugly switch in the TLVFactory.
2. Is there a way to make the TLVFactory::create a static member of TLVObject?
3. Any other code improvement suggests, perhaps regarding casts.

tlv2.cpp

#include <iostream>
#include <iomanip>
#include <fstream>
#include <cstdint>
#include <string>
#include <vector>
#include <memory>
#include <stdexcept>

class TLVObject
{
public:
    enum TLV_TYPE {UNDEFINED = 0, BYTE, WORD, DWORD, QWORD, STRING, BLOB};
    TLVObject(TLV_TYPE type) :
        m_type{type}
    {}
    virtual ~TLVObject() {}
    virtual size_t len() const { return 0; }
    virtual std::ostream &write(std::ostream &out) const { 
        writeTag(out);
        writeLen(out);
        writeData(out);
        return out;
    }
    virtual std::ostream &writeTag(std::ostream &out) const { 
        return out.put(m_type); 
    }
    virtual std::ostream &writeLen(std::ostream &out) const {
        return out.put(len());
    }
    virtual std::ostream &writeData(std::ostream &out) const {
        return out;
    }
    virtual std::istream &read(std::istream &in) {
        readTag(in);
        readLen(in);
        readData(in);
        return in;
    }
    virtual std::istream &readTag(std::istream &in) { 
        char ch;
        in.get(ch);
        m_type = static_cast<TLV_TYPE>(ch);
        return in;
    }
    virtual std::istream &readLen(std::istream &in) { return in; }
    virtual std::istream &readData(std::istream &in) { return in; }
private:
    TLV_TYPE m_type;
};

class TLVByte : public TLVObject
{
public:
    TLVByte(uint8_t value) :
        TLVObject{BYTE},
        m_value{value}
    {}
    TLVByte() : TLVByte{0} {}
    virtual ~TLVByte() {}
    virtual size_t len() const { return sizeof(m_value); }
    virtual std::ostream &writeData(std::ostream &out) const { 
        return out.put(m_value); 
    }
    virtual std::istream &readLen(std::istream &in) {
        if (len() != static_cast<size_t>(in.get())) {
            std::out_of_range("TLV len error");
        } 
        return in;
    }
    virtual std::istream &readData(std::istream &in) { 
        m_value = in.get(); return in; 
    }
private:
    uint8_t m_value;
};

class TLVWord : public TLVObject
{
    public:
    TLVWord(uint16_t value) :
        TLVObject{WORD},
        m_value{value}
    {}
    TLVWord() : TLVWord{0} {}
    virtual ~TLVWord() {}
    virtual size_t len() const { return sizeof(m_value); }
    virtual std::ostream &writeData(std::ostream &out) const { 
        return out.put(m_value >> 8).put(m_value & 0xff);
    }
    virtual std::istream &readLen(std::istream &in) {
        if (len() != static_cast<size_t>(in.get())) {
            std::out_of_range("TLV len error");
        } 
        return in;
    }
    virtual std::istream &readData(std::istream &in) { 
        m_value = in.get() << 8; 
        m_value |= in.get();
        return in; 
    }
private:
    uint16_t m_value;
};


class TLVString : public TLVObject
{
public:
    TLVString(std::string str) :
        TLVObject{STRING},
        m_value{str}
    {}
    TLVString() : TLVString("") {}
    virtual ~TLVString() {}
    virtual size_t len() const { return m_value.size(); }
    virtual std::ostream &writeData(std::ostream &out) const { 
        return out << m_value;
    }
    virtual std::istream &readLen(std::istream &in) {
        m_value.reserve(in.get());
        return in;
    }
    virtual std::istream &readData(std::istream &in) { 
        for (int i = m_value.capacity(); i; --i) {
            m_value += in.get();
        }
        return in; 
    }
private:
    std::string m_value;
};

class TLVFactory
{
public:
    static std::unique_ptr<TLVObject> create(TLVObject::TLV_TYPE type) {
        switch (type) {
            case TLVObject::BYTE: 
                return std::make_unique<TLVByte>();
                break;
            case TLVObject::WORD: 
                return std::make_unique<TLVWord>();
                break;
            case TLVObject::STRING: 
                return std::make_unique<TLVString>();
                break;
            case TLVObject::DWORD: 
            case TLVObject::QWORD: 
            case TLVObject::BLOB:
            default:
                std::out_of_range("Can't construct object of that type");
        }
        return std::make_unique<TLVObject>(TLVObject::UNDEFINED);
    }
};

class TLVCollection : public std::vector<std::unique_ptr<TLVObject>> {
public:
    void write(std::ostream &out) const {
        for (const auto &item : *this) {
            item->write(out);
        }
    }
    void read(std::istream &in) {
        while (in.good() && in.peek() > 0) {
            push_back(TLVFactory::create(static_cast<TLVObject::TLV_TYPE>(in.peek())));
            back()->read(in);
        }
    }
};

Simple test code

int main()
{
    TLVCollection vec;
    vec.push_back(std::make_unique<TLVByte>(TLVByte{7}));
    vec.push_back(std::make_unique<TLVString>(TLVString{"banana"}));
    vec.push_back(std::make_unique<TLVWord>(TLVWord{0x1234}));
    std::ofstream out("tlvtest.bin");
    vec.write(out);
    out.close();

    TLVCollection v2;
    std::ifstream in("tlvtest.bin");
    v2.read(in);
    in.close();
    v2.write(std::cout);
}

Results

When I run the program and pipe its output through xxd which simply converts the binary input to a hexadecimal representation, I get this:

./tlv2 |xxd
00000000: 0101 0705 0662 616e 616e 6102 0212 34    .....banana...4

Answer 1

Here are a few suggestions.

Use const where appropriate

In your constructors you simply copy the passed-in value without mutating it. It should be declared const since it's not mutated.

Don't allow invalid objects to be constructed

I don't understand the point of allowing a caller to create an undefined TLV type. It has 0 length, so there's nothing to write or read. It would be better to simply not allow such a thing to even be constructed. I'd remove the UNDEFINED value from the enum. (It might be worth it to make the constructor for the base class protected so it can't be directly instantiated outside of a subclass.)

Use pure virtual methods where appropriate

If TLVObject made virtual the methods which must be overridden by subclasses, the compiler will stop a caller from constructing the base object. So I'd make len(), writeData(), readLen(), and readData() all pure virtual:

virtual size_t len() const = 0;
...
virtual std::ostream &writeData(std::ostream &out) const = 0;
...
virtual std::istream &readLen(std::istream &in) = 0;
virtual std::istream &readData(std::istream &in) = 0;

Use override on overridden methods

When you override a method in a subclass, you should mark it as override so the compiler knows your intent and will warn you if the method signature ever changes in the base class or if you spell it wrong in a subclass. So it would look like this:

class TLVByte : public TLVObject
{
...
    virtual size_t len() const override { return sizeof(m_value); }
    virtual std::ostream &writeData(std::ostream &out) const override { 
        return out.put(m_value); 
    }
    virtual std::istream &readLen(std::istream &in) override {
        if (len() != static_cast<size_t>(in.get())) {
            std::out_of_range("TLV len error");
        } 
        return in;
    }
    virtual std::istream &readData(std::istream &in) override { 
        m_value = in.get(); return in; 
    }

...

Factory method

Overall your factory method looks completely reasonable. I originally though, "There's no reason it couldn't be a public static method inside of TLVObject. You'd simply need to forward declare the subclasses":

class TLVByte;
class TLVWord;
class TLVString;

class TLVObject
{
public:
    enum TLV_TYPE {UNDEFINED = 0, BYTE, WORD, DWORD, QWORD, STRING, BLOB};
    static std::unique_ptr<TLVObject> create(TLVObject::TLV_TYPE type) {
        switch (type) {
            case TLVObject::BYTE: 
                return std::make_unique<TLVByte>();
                break;
            case TLVObject::WORD: 
                return std::make_unique<TLVWord>();
                break;
            case TLVObject::STRING: 
                return std::make_unique<TLVString>();
                break;
            case TLVObject::DWORD: 
            case TLVObject::QWORD: 
            case TLVObject::BLOB:
            default:
                std::out_of_range("Can't construct object of that type");
        }
        return std::make_unique<TLVObject>(TLVObject::UNDEFINED);
    }
...

However, I was wrong. The above won't work because you're trying to use a not-yet fully described type as the argument to a template (std::unique_ptr<T>), and that's not going to work.

Another option is to make it a free-standing function. There's no reason it needs to be in a class by itself, though there's no problem with doing it that way.

As you probably guessed from the above mistake, I'm no expert at templates. So a question - would it be possible to do something like make the create() method a templated function? Something like:

template<T>
static unique_ptr<T> create()
{
    return std::make_unique<T>();
}

Or even just get rid of the factory function and create them just using std::make_unqiue<Whichever>();?

Also, the last return statement for your create() will never be reached since you have a default case. And as mentioned above, it shouldn't be necessary because you shouldn't allow construction of an undefined object.