5

I am working on a networking program and designing a Linux server using C++. This is quite straightforward to design the basic structure. I have a packet definition with a header that has a fixed size.

typedef enum{
     PACKET_LOGIN_REQ = 1,
     PACKET_LOGIN_RES,
     PACKET_STORE_REQ,
     PACKET_STORE_RES
}PACKET_TYPES;

typedef struct {
     PACKET_TYPES type;
     short bodySize,
     long long deviceId
}HEADER;

.
.

/*more definitions here*/

typedef struct{
     HEADER head;
     union BODY{
          LOGIN_REQ loginReq;
          LOGIN_RES loginRes;
          .
          .
          more types
     }
}

Whenever I added more packet types I would have to modify the switch statement to add more cases to handle the newly added packets.

I am using the union type so I don't have to change the entire packet structure. Instead I can add the newly added packet types into the union structure.

However, when I am trying to parse the raw data to put into the packet using a switch statement then I have to add each switch statement every time.

I think this is not a good design pattern and I was wondering how I can design the structure in a more flexible way.

Is there a better way to handle this (better design pattern)? What about related tutorials or references?

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
codereviewanskquestions
  • 13,460
  • 29
  • 98
  • 167
  • If you are using C++, you can and should lose all those typedefs. And have you heard about inheritance and polymorphism? –  Jun 15 '11 at 08:01

4 Answers4

7

You should use classes and instead of switch use a polymorphic call.

Something like that (pseudo-codish):

class Packet{
public:
   Packet();
   virtual ~Packet();
   virtual HandleMe()=0;
protected:
   // fields 
}

PacketTypeOne: public Packet{
public:
  virtual HandleMe(); // implementation for type 1
}

PacketType2: public Packet{
public:
  virtual HandleMe(); // implementation for type 2
}

PacketTypeX: public Packet{
public:
  virtual HandleMe(); // implementation for type X
}

and where you get the packets:

...
Packet* newPacket = PacketFactory::CreateNewPacket(packetTypeX); // factory will create
              // the right instance.
...
// now handle it:
newPacket->HandleMe();
...

Done.

littleadv
  • 20,100
  • 2
  • 36
  • 50
3

Since your code is C style and not C++ style code, I will give you the C answer.

Use function pointers. Something like:

typedef int (*ParseFunc)(const char *, len, struct MyStruct);

typedef struct PacketParser_
{
    PACKETType type;
    ParseFunc parseFunc;
} PacketParser;

const PacketParser[] parser = 
{
   { PACKET_LOGIN_REQ, LoginReqParser }
   { PACKET_LOGIN_RES, LoginResParser }
   .
   .
   .
}

You then iterate through all the packet types and call the corresponding function. Whenever you get a new packet type, you just add another line to your PacketParser function.

If you are using C++ you are better off using polymorphism.

doron
  • 27,972
  • 12
  • 65
  • 103
  • +1 - although the question is tagged C++ - your answer is a perfect and elegant solution for a C implementation of the same use case, I like it. – littleadv Jun 15 '11 at 09:32
1

In most cases, you should avoid unions. This is one of those cases :-)

The answer, as noted, is to use polymorphism. The canonical way to convert switch statements to polymorphism is using the Visitor design pattern.

While normal polymorphism, using virtual methods may help you, there is a problem. virtual methods give you a dispatch to the correct method based on a single object (the Packet). But, you also depend on what action to take. For example, if you have many switch statements, and not one, then the Visitor pattern will be preferable.

Community
  • 1
  • 1
Gilad Naor
  • 20,752
  • 14
  • 46
  • 53
0

You might want to ...

Essentially, you find a good super class, and for each typecode create a distinct subclass.

You also want to buy a copy of "Refactoring: Improving the design of existing code." , Fowler, Martin (1999) / Addison Wesley..

Sebastian Mach
  • 38,570
  • 8
  • 95
  • 130