Polymorphic TLV serialization

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:

• I'd like to get rid of the big ugly switch in the TLVFactory.

• Is there a way to make the TLVFactory::create a static member of TLVObject?

• Any other code improvement suggests, perhaps regarding casts.

tlv2.cpp

```
#include
#include
#include
#include
#include
#include
#include
#include

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(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.

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(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 create(TLVObject::TLV_TYPE type) {
        switch (type) {
            case TLVObject::BYTE: 
                return std::make_unique();
                break;
            case TLVObject::WORD: 
                return std::make_unique();
                break;
            case TLVObject::STRING: 
                return std::make_unique();
                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::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), 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
static unique_ptr create()
{
    return std::make_unique();
}

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

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.

StackExchange Code Review Q#115584, answer score: 3