Good design question

A

Andrea Crotti

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
 
M

Michael Doubez

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

Andrea Crotti

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.
 

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Serialization 10
Inheritance question 12
operator== 12
Vector of types 6
C++ OO design question 8
overloading operator << design question 2
feedback on code design 23
C++, a good starting language? 36

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,483
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top