Designfrage - Guter Threadtitel schwer...

314159265358979

Also, ich habe ne Designfrage.

Ich bekomme an einem Socket Pakete rein. Es gibt über 50 verschiedene Arten von Paketen. Welches Paket das nun ist, hängt von einem Typ-Byte ab. Die Pakete bestehen aus diesem Byte und danach andere binäre Daten. Momentan lese ich ein Paket so:

std::unique_ptr<packet> read_packet(socket& sock)
		{
			int8_t id;
			sock >> id;

			std::unique_ptr<packet> p = create_server_packet(id);
			p->read(sock);
			return p;
		}

Wobei create_server_packet() dann so aussieht:

std::unique_ptr<packet> create_server_packet(int8_t id)
		{
			switch(id)
			{
			case 0:
				return make_unique<packet_000_keep_alive>();

			case 1:
				return make_unique<packet_001_login_request_server>();

			.
			.
			.

			default:
				throw packet_error("invalid packet id!");
			}
		}

Die Funktion create_server_packet() wirkt auf mich wie schlechtes Design, vor allem muss ich da alle neuen Pakete eintragen. Andererseits hat sie nicht den Overhead einer map<id, creator_function>

Wie könnte ich das verbessern? Oder ist das brauchbar?

Grüße,
PI

otze

Wenn es das einzige switch/case Konstrukt der Form ist, dann würde ich da erstmal nichts ändern wollen. Ich vermute aber auch, dass es sehr unwahrscheinlich ist, dass es mal ein paket nr 51 geben wird.

314159265358979

Da es sich um eine Minecraft-Library handelt, werden ziemlich sicher mal neue Pakete dazukommen.

Nexus

Sind die IDs konsekutiv? Dann könntest du statt einer std::map einen std::vector verwenden. Und sonst gäbs auch noch std::unordered_map , wenn schneller Zugriff wichtig ist.

314159265358979

Ich nehme an, konsekutiv bedeutet fortlaufend? Falls ja, sind die das nicht. Ich weiß auch nicht, wie gut ein Compiler die Sprünge optimieren kann...

Nexus

Dann nimm std::map oder std::unordered_map . Bist du sicher, dass der Overhead des Paket-Dispatchs bei deiner Netzwerkanwendung nicht vertretbar ist?

314159265358979

Es wird vermutlich keine Rolle spielen. Bei der map müsste ich dann ja jede Paketklasse irgendwie registrieren, oder? Da ist die switch-Variante doch irgendwie einfacher, scheint mir zumindest.

Nexus

Du kannst ja eine Map nehmen, die int8_t auf Funktionszeiger oder std::function abbildet, wobei das entsprechende Paket zurückgegeben wird.

Rhombicosidodecahedron

314159265358979 schrieb:

Ich nehme an, konsekutiv bedeutet fortlaufend? Falls ja, sind die das nicht. Ich weiß auch nicht, wie gut ein Compiler die Sprünge optimieren kann...

Vielleciht von Interesse: http://www.codeproject.com/KB/cpp/switch.aspx

TyRoXx

Was passiert denn mit den Paketen, wie sieht packet aus? Sind diese Klassen wirklich notwendig?

314159265358979

Ich habe mein Design gerade nochmal etwas geändert, es gibt nun 2 Basisklassen: client_packet und server_packet. Diese sehen wie folgt aus:

struct client_packet
		{
			void write(socket& sock) const;
			virtual ~client_packet() {}

			const int8_t id;

		protected:
			client_packet(int8_t id)
				: id(id)
			{}

		private:
			virtual void do_write(socket&) const = 0;
		};

		struct server_packet
		{
			virtual ~server_packet() {}

			const int8_t id;

		protected:
			server_packet(int8_t id)
				: id(id)
			{}
		};

Von diese abgeleitet gibt es dann für jeden Pakettyp eine Klasse. Teilweise unterscheiden sich jedoch Client von Serverpaketen, deshalb trenne ich das. Hier ein Beispiel:

namespace detail
		{
			struct packet_013_player_position_and_look
			{
				packet_013_player_position_and_look() = default;

				packet_013_player_position_and_look(double x, double y, double stance, double z, float yaw, float pitch, bool on_ground)
					: x(x)
					, y(y)
					, stance(stance)
					, z(z)
					, yaw(yaw)
					, pitch(pitch)
					, on_ground(on_ground)
				{}

				double x;
				double y;
				double stance;
				double z;
				float yaw;
				float pitch;
				bool on_ground;
			};
		}

		class packet_013_player_position_and_look_client : public client_packet, public detail::packet_013_player_position_and_look
		{
			virtual void do_write(socket& sock) const
			{
				sock << x << y << stance << z << yaw << pitch << on_ground; // hier wird stance nach y geschrieben
			}

		public:
			packet_013_player_position_and_look_client(double x, double y, double stance, double z, float yaw, float pitch, bool on_ground)
				: client_packet(13)
				, detail::packet_013_player_position_and_look(x, y, stance, z, yaw, pitch, on_ground)
			{}
		};

		struct packet_013_player_position_and_look_server : public server_packet, public detail::packet_013_player_position_and_look
		{
			packet_013_player_position_and_look_server(socket& sock)
				: server_packet(13)
			{
				sock >> x >> stance >> y >> z >> yaw >> pitch >> on_ground; // hier wird stance vor y eingelesen
			}
		};

Zwischen den Paketen gibt es wiederum Unterschiede den Aufbau betreffend. Eine vollständige Liste ist hier: Klick

314159265358979 schrieb:

Die Funktion create_server_packet() wirkt auf mich wie schlechtes Design, vor allem muss ich da alle neuen Pakete eintragen. Andererseits hat sie nicht den Overhead einer map<id, creator_function>

Wie könnte ich das verbessern? Oder ist das brauchbar?

Ich finde das so schon OK. Der Punkt ist ja, irgendwo brauchst du so eine Fallunterscheidung - ob die jetzt "direkt" im Sourcecode steht oder "indirekt" in Form von Config-Files dynamisch von einem Server geladen wird. Wichtig ist nur, dass mögliche spätere Änderungen möglichst einfach und lokal ablaufen. Das sehe ich gegeben, du legst eine kleine neue Klasse für den Pakettyp an und fügst einen Eintrag in dem switch-Konstrukt hinzu.

Die Wahl der Technik ist nochmal ein anderes Ding. Also ob switch, map oder sonstwas. Und ob Prototype-Pattern, Factory und Funktionszeiger ja/nein usw. Wegen den nur 50 Einträgen spielt die Performance hier wohl keine Rolle, da ist switch doch übersichtlich und leicht verständlich.

Insgesamt finde ich das gut, ja.

TyRoXx

Wenn man dann also als Benutzer ein unique_ptr<server_packet> hat, was macht man dann damit? Wie bekommt man den Pakettyp heraus?

314159265358979

Naja, der Typ steht in .id drinnen. Irgendwie muss man dann halt nochmal ein switch machen.

TyRoXx

314159265358979 schrieb:

Naja, der Typ steht in .id drinnen. Irgendwie muss man dann halt nochmal ein switch machen.

Wirkt auf mich unnötig kompliziert und dazu noch langsam. Warum nicht einfach ein Interface, das für jeden Pakettyp beim Empfangen eine Methode hat?
Zum Senden pro Paket eine Funktion/Klasse/whatever ganz ohne Polymorphie.