Re: Good design question

From:

Michael Doubez <michael.doubez@free.fr>

Newsgroups:

comp.lang.c++

Date:

Tue, 5 Apr 2011 01:55:07 -0700 (PDT)

Message-ID:

<c4e04167-7846-4c12-9696-08e596a9ef8f@e26g2000vbz.googlegroups.com>

On 5 avr, 09:54, Andrea Crotti <andrea.crott...@gmail.com> wrote:

I've been working on this project for quite a long time, and still I'm
not satisfied by the basic design and can't find a better way.

Instead of writing what I've done I'll write what I would like to have
(as a programmer-user) and then how I tried to solve it.

I have different types of packets, with a base header and some extra
fields depending on what the packet is for, like.

Packet
- field1
- field2

+ Packet2
  - field3

and so on.

I would like to be able to:

Packet pkt(field1, field2);
sendStream(pkt.toNetworkStream());

and toNetworkStream is defined only once also for the derived packets.

I would like also that if I do

Packet *pkt = new Packet2(Packet1&, field3);
pkt.get("field3");

works too, without having to do dynamic_cast.

So what I have now is something like this:

--8<---------------cut here---------------start------------->8---
// class for managing the low level packets
class Packet : public Serializable
{
public:
    Packet() {}
    Packet(PACKET_TYPE);
    PACKET_TYPE type;
    std::vector<Serializable *> fields;

    virtual void toBuffer(char *, int *) const;
    // this function can't be overriden
    Stream toStream() const;
    virtual void setFields() {}};

--8<---------------cut here---------------end--------------->8---

Where a serializable object is something that is able to generate a
stream of chars from itself.

The toStream() works nicely, since every object sets a vector of the
pointers to its fields.

But the initialization of a packet for example is not so nice:
--8<---------------cut here---------------start------------->8---
CoordReq::CoordReq(const PadNodeID& _id_dest,
                   const PadCoordinate& _coord_dest,
                   const PadNodeID& _id_src,
                   const PadCoordinate& _coord_src,
                   const PadNodeID& _to_find)
: MultiCastPacket(_id_dest, _coord_dest, _id_src, _coord_src),
    to_find(_to_find)
{
    setFields();

}

void CoordReq::setFields(){
    type = COORD_REQ;
    fields.push_back(&to_find);}

--8<---------------cut here---------------end--------------->8---

Here MultiCastPacket has the four fields to initialize and then I add
only one extra field.

About the second problem instead I didn't really find a solution, and
I
ended up doing a dynamic_cast on the pointer to make sure I pass the
right pointer to the functions handling it.

Ending up in something like:
--8<---------------cut here---------------start------------->8---
    switch (pkt->type) {
    case ADDRESS_UPDATE:
        handleAddressUpdate(dynamic_cast<AddressUpdate *>(pkt));
        break;
--8<---------------cut here---------------end--------------->8---

I thought that a possible way was to substitute

std::vector<Serializable *> with

std::map<std::string, Serializable *>

And then instead of
obj.variable, only use obj.getField("field_name").

In this way I should solve my problems, but

1. the order of the fields might be messed up
2. I'm quite sure there are better ways even if I don't find them

Any advice?

I also have this kind of issues and so far, I have not found a totally
satisfactory solution.
What we did is that we basically split the resonsibilities:
  - Packet: for holding the fields and values specific to each kind of
message
  - Reader: for unserialising packet
  - Writer: for serialising packet

For each specific packet, a awk script (or by hand) generates a header
file using macros for defining the fields or the packet. Then we use,
macro machinery with a little template to generate the relevant
classes and enums for addressing the fields. We try to keep it simple
in order to avoid debuging into the macros.

Example:

class Packet
{
public:
    // ...
    virtualSerialisable* getField(int) = 0;
};
------
// Packet1Fields.h
FIELD( Foo, foo_type, serialisation_type, ....)
FIELD( Bar, bar_type, serialisation_type, ....)
-------
// Packet1.h
class Packet1
{
public:
  enum FieldId {
#define FIELD( _name, ...) _name,
#include Packet1Fields.h
#undef FIELD
  };

public:
  virtualSerialisable* getField(int f)
  {
   switch( f ){
#define FIELD( _name, ...) case _name: return _field_##_name;
#include Packet1Fields.h
#undef FIELD
    default: return NULL;
   }
  }

private:
// defien field content
#define FIELD( _name, _type, ...) _type _field_##_name;
#include Packet1Fields.h
#undef FIELD
};

It is a bit rough but it is the idea. I find it also a good idea to
define a visitor on all fields.

I tried a pure template solution but it was so ugly and unwieldy that
I am sure nobody would have accepted it (and you cannot do efficient
switch/case with macro).

--
Michael