Good design question

Discussion in 'C++' started by Andrea Crotti, Apr 5, 2011.

  1. 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?
    Thanks,
    Andrea
     
    Andrea Crotti, Apr 5, 2011
    #1
    1. Advertising

  2. On 5 avr, 09:54, Andrea Crotti <> 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
     
    Michael Doubez, Apr 5, 2011
    #2
    1. Advertising

  3. On Apr 5, 10:55 am, Michael Doubez <> wrote:

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



    Thanks for the answer :)
    But Macro and AWK are not really what I wanted, then I'll keep my
    solution instead...

    The Writer for me is very simple, since every field knows how to
    convert itself to a stream, and also the Reader is quite
    straightforward.

    The only problem as I said is the impossibility to use virtual
    functions directly with t his approach.

    I think the map would work just fine, the only thing is that I have to
    fetch the fields with

    obj.getField("field_name") instead of obj.field_name and that the
    order could be messed up (but since reading and writing use the same
    order it should not be a big deal.
     
    Andrea Crotti, Apr 5, 2011
    #3
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. antoine
    Replies:
    7
    Views:
    499
    Rhino
    Mar 3, 2005
  2. sikka noel
    Replies:
    8
    Views:
    437
    Mike Wahler
    Aug 5, 2003
  3. vlsidesign
    Replies:
    26
    Views:
    1,043
    Keith Thompson
    Jan 2, 2007
  4. Cliff  Martin
    Replies:
    1
    Views:
    3,081
    Larry Smith
    Jan 31, 2007
  5. Bazza Formez
    Replies:
    1
    Views:
    367
    =?Utf-8?B?UGV0ZXIgQnJvbWJlcmcgW0MjIE1WUF0=?=
    Aug 9, 2007
Loading...

Share This Page