2
\$\begingroup\$

I want to let you look through my NetHandler which is basically the component which gets notified when a full packet arrived (not like tcp-packet but my own type of packets) and handles it.

First of all, the handle method which gets called from the networking

void NetHandler::handle(int id, char* data, int len) {
#define P(ID, TYPE) case ID: try_handle<TYPE>(data, len); break
    switch (id) {
        P(0, game::networking::packets::Hello);
        P(1, game::networking::packets::Disconnect);
        P(2, game::networking::packets::Chat);
    default:
        unhandled(id);
    }
#undef P
}

Yeah... As you can see, a little define to create a case for me. unhandled just prints out an error.

Next, the try_handle<>()

template<typename T>
void try_handle(char* data, int len) {
    T packet;
    if (!packet.ParseFromArray(data, len)) {
        bad_packet(packet.GetTypeName(), len);
        return;
    }
    handle(packet);
}

It creates the actual corresponding Packet and parses it (I use google protobuf). bad_packet, again, just prints an error. Now the handle

#define STUB(TYPE) virtual void handle(TYPE& type) { unhandled(TYPE::default_instance().GetTypeName()); }
    STUB(game::networking::packets::Hello)
    STUB(game::networking::packets::Disconnect)
    STUB(game::networking::packets::Chat)
#undef STUB

I create a virtual method for every packet-type which calls unhandled. In my application I can now subclass this NetHandler and override the handler-methods for all packets I expect.

Now the other direction, send packets. I actually wanted a single send-method which takes every possible packet and just sends it. But it needs to send the id along with it, so I created a PacketRegistry

class PacketRegistry {
public:
    PacketRegistry() {
        for (int i = 0; i < 256; i++)
            messages[i] = nullptr;
        reg<game::networking::packets::Hello>(0);
        reg<game::networking::packets::Disconnect>(1);
        reg<game::networking::packets::Chat>(2);
    }

    template<typename T>
    void reg(int id) {
        messages[id] = T::default_instance().GetDescriptor();
    }

    int getId(const google::protobuf::Message& message) {
        const google::protobuf::Descriptor* ptr = message.GetDescriptor();
        for (int i = 0; i < 256; i++) {
            if (messages[i] == ptr)
                return i;
        }
        return -1;
    }

    const google::protobuf::Descriptor* messages[256];
};

As you can see, this is the third time I need to add every packet manually.

I tried to come up with some template-magic, but I'm not that experienced yet. Something like

//in NetHandler
template<typename T>
void handle(T& packet) { unhandled(T::default_instance().GetTypeName()); }

//In a subclass
template<>
void handle<game::networking::packets::Chat>(game::networking::packets::Chat& chat) {   }

unfortunately doesn't work because... well... templates and virtual don't really like each other.

Any ideas how I can let the template-magic do more of the dirty work instead of defining methods, cases, and so on for every existing packet?

| improve this question | | | | |
\$\endgroup\$
1
\$\begingroup\$

Few suggestions:

  • Add/use inheritance for packet type (assuming you have something like game::networking::packets::PacketTypeBase that all packet types inherit from. Have GetDescriptor()/GetTypeName()/handle() on this base class

  • Create a factory to go from int id to packet Type. Something like: game::networking::packets::PacketTypeBase make_packet(int id);

  • Add a virtual function handleInput(char* data, int len) so your NetHandler::handle simply becomes return make_packet(id).handleInput(data,len). The default can be handled by base class or separate UnknownPacket child class.

I would recommend using inheritance on packet type, not NetHandler. If you need flexibility, you can even pass a functor into the ctor of NetHandler if you want different work done on the PacketType by each instance of NetHandler

In general try to replace switch statement with inheritance and virtual functions. Factory/maker function comes in handy even during testing. Common functionality can be put in base class so you can avoid duplicate code (not need macros) & still do packet specific work.

Instead of having a hard-coded list of 256 (avoid magic numbers), use a std::map / std::unordered_map/HashMap so find / lookup will be faster in getId (use a data structure that optimizes your most common case)

Hope this helps.

| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ Hey, thanks for the feedback. Some notes though: 1.: I use google-protobuf, so all my messages are subclasses of protobuf's Message-class. I don't think I can add my own methods in there. 2. and 3.: So the package handles itself? Well, would probably be nice iff I could add custom methods to the packets but I don't want to mess with the packets sourcecode after compiling it from .proto files. \$\endgroup\$ – tkausl May 4 '16 at 0:09
  • \$\begingroup\$ You could add your own light wt wrapper around google-protobuf for what you need. Having your own class will also give you better control over your code in terms of future enhancements & maintenance (like you could choose a different serialization library for networking in the future and not have to change every interface) \$\endgroup\$ – Chintan May 4 '16 at 0:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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