1
\$\begingroup\$

I have created an NBT format reader (NBT is a binary serialization format that Minecraft uses to store its data). I made a node-like class that can be a data entry or the root of a subtree (that's how NBT is defined). The public interface of it currently looks like this:

class Tag
{
    Tag();

    void read(std::istream& stream);
    void write(std::ostream& stream);

    void print(int indent = 0);

    int8_t byteValue() const;
    int16_t shortValue() const;
    int32_t intValue() const;
    int64_t longValue() const;
    float floatValue() const;
    double doubleValue() const;
    ByteArray byteArrayValue() const;
    IntArray intArrayValue() const;
    UTF8String stringValue() const;
    std::vector<Tag> children() const;

    UTF8String name() const;
};

Is this the right design though? Originally I wanted to do the *Value methods with templates, but the problem is that the value of a Tag is dynamic, so that does not play nicely with the compile-time genericity.

Is it a blasphemy not to make the various types derived classes? I couldn't really take advantage of a virtual value method because the return types would need to differ. Is there a better way to implement a data structure like this? The aim is to make it easy to use and fast.

\$\endgroup\$
  • \$\begingroup\$ I think its hard to judge without some idea (sample code?) of how you are going to actually use it. \$\endgroup\$ – Winston Ewert Mar 15 '12 at 22:34
  • \$\begingroup\$ I intend create a library with it, so I can't really tell. It's more like an API designer's dilemma. It is for loading Minecraft data files which contain trees of this data structure. The average use case is to iterate over the nodes, add/remove subtrees and values. So basically everything you can imagine with a tree. \$\endgroup\$ – Tamás Szelei Mar 15 '12 at 22:39
2
\$\begingroup\$

Couple of things I noticed:

The serialize members:

void read(std::istream& stream);
void write(std::ostream& stream);

I don't particular mind them but I also would prefer to have operator<< and operator>> defined (Note you can simply make these call write/read respectively).

With the stream iterators defined this becomes less useful.

void print(int indent = 0);

The following seem like very simialer.

int8_t byteValue() const;
int16_t shortValue() const;
int32_t intValue() const;
int64_t longValue() const;
float floatValue() const;
double doubleValue() const;
ByteArray byteArrayValue() const;
IntArray intArrayValue() const;
UTF8String stringValue() const;

I think we can replace all of them with:

template<typename T>
T const& get() const;

I like this better as we do not need to guess that byte maps to int8_t or short maps to int16_t as we can explicitly get the type we want:

 int8_t  val = t.get<int8_t>();

But there is already something very similar defined in boost:

boost::any
\$\endgroup\$
1
\$\begingroup\$

Originally I wanted to do the *Value methods with templates, but the problem is that the value of a Tag is dynamic, so that does not play nicely with the compile-time genericity

Using byteValue(), shortValue() etc doesn't solve this problem completly. Let's consider this code:

Tag t;
t.setIntValue(10);
//...... some time later
uint_8 a = t.byteValue(); // Ok (1)
//...... some more time later
int array[10] = {/*....*/}
t.setArray(array);
//...calling (1) again
uint_8 a = t.byteValue(); // ??

In any case you have to code convert logic, which converts value from one type to another. The only difference is form this logic are implemented - either methods like byteValue(), shortValue() or template methods.

I don't think it's a good idea to use name-specific methods to obtain values. Template methods are looked quite more suitable.

class Tag
{
   template<typename T>
   getValue() 
   {/*default function
     here you can implement default convert logic. For example you can forbid convertion
     to non-specified types using boost::enable_if or you own implemented SFINAE methods
     or you can convert non-specified Types to int type.*/
   };

   template<>
   getValue()<int> {return to-int-converted-value}

   template<>
   getValue()<ByteArray> {return to-byteArray-converted-value}

   template<>
   getValue()<someExoticType> // this convertion might be forbid with using boost::disable_if....
   {}
};

Now, client code looks like this:

Tag t;
t.setIntValue(10);
uint_t a = t.getValue();
int array[10] = {/*....*/}
t.setArray(array);
uint_t b = t.getValue();

someExoticType ex = t.getValue() // error, no convertion
\$\endgroup\$
  • 1
    \$\begingroup\$ Unfortunately that will not work. t.getValue() The compiler does not know which version of getValue() to call because template resolution does not use the return type for resolution. So you must explicitly specify the template type t.getValue<int>() \$\endgroup\$ – Martin York Mar 16 '12 at 7:50
  • \$\begingroup\$ @fogbit, you are right about the problems with multiple getValue methods, but I specifically took care of that by asserting the TagType (an enum) and the size of the payload. My strategy is that set will set the TagType to whatever it wants and there is only on payload member (a vector of bytes). So in your first example you will get an assertion error when you try to call byteValue() again. \$\endgroup\$ – Tamás Szelei Mar 16 '12 at 8:30
  • \$\begingroup\$ As @Loki Astari said, automatic template parameter deduction only works on templates. I tried this and I chose the current solution because a) it's cleaner (easier to understand for other people, which is one of my intentions) b) You have to write out the type anyway. \$\endgroup\$ – Tamás Szelei Mar 16 '12 at 8:32
  • \$\begingroup\$ @TamásSzelei: Sounds like you have re-invented boost::any \$\endgroup\$ – Martin York Mar 16 '12 at 13:58
  • \$\begingroup\$ hmm, that's not a problem to me though :) Is it possible to allow only certain types to be stored in boost::any? \$\endgroup\$ – Tamás Szelei Mar 16 '12 at 15:14

Not the answer you're looking for? Browse other questions tagged or ask your own question.