Designfrage: dynamic_cast hier angebracht?

[Roger Wilco]

Hmm, das klingt alles logisch. Eine Klasse macht Sinn.

Die Basisklasse benötige ich jedoch auch noch für den gemeinsamen Containser, in dem alle Requests bis zu Ihrer Beantwortung gespeichert werden sollen. Die sollten eigentlich alle in einen STL-Container.

Ich müsste die dann in separate Container stecken (typenrein) und bei einem Response alle Container nach dem entsprechenden Request durchsuchen. Das würde aber auch bedeuten, dass bei einem neuen Request-Typ ein neuer Container her muss und ich muss diesen dann zusätzlich auch durchsuchen.

[dot]

Roger Wilco schrieb:

Die Basisklasse benötige ich jedoch auch noch für den gemeinsamen Containser, in dem alle Requests bis zu Ihrer Beantwortung gespeichert werden sollen. Die sollten eigentlich alle in einen STL-Container.

Gut, das ist ein Argument für eine Basisklasse, ist aber unabhängig vom Rest. Die Response muss trotzdem nicht wirklich von Außen zugeführt werden, oder?

[Roger Wilco]

dot schrieb:

Die Response muss trotzdem nicht wirklich von Außen zugeführt werden, oder?

Nein, muss nicht. Würde dann so aussehen:

class Request //Basisklasse für alle Anfragen
{ 
   ...
   void Serialze(DataStream& stream) const =0;
   void ProcessRespond(DataStream& stream)  const =0; //Antwort parsen und bearbeiten
   ...
};

[hustbaer]

Ich denke mir:
* mach dir mal wegen des dynamic_cast keinen Kopf
* dass der Handler (ProcessRespond) in der Request Klasse zu finden ist, sieht mir irgendwie ziemlich komisch/fischig aus
* meinst du nicht vielleicht Response?

[Roger Wilco]

hustbaer schrieb:

* mach dir mal wegen des dynamic_cast keinen Kopf

Nein? Performance (zweitranging)? Indiz für schlechtes Design?

hustbaer schrieb:

* dass der Handler (ProcessRespond) in der Request Klasse zu finden ist, sieht mir irgendwie ziemlich komisch/fischig aus

Stimme ich Dir zu. Aber ich wollte, dass man nur eine Klasse hinzufügen muss, wenn man ein neuen Request-Typ hinzufügen möchte. Ich finde einfach keinen besseren Ansatz.

Ein anderer Versuch:

//Client.h:

#include <functional>
#include <map>
#include "Request.h"
#include "Response.h"

class Client
{
    ...
public:
    typedef std::tr1::function<void(const Request& request, const Response& response)> ProcessResponseFunc;
    // fügt eine Funktion zum Auswerten von Antworten hinzu
    void RegisterProcessResponseFunction(const ProcessResponseFunc& func);

    // ProcessResponse überprüft die Antwort auf Gültigkeit (Header-Abgleich)
    // und stößt ggf. Folgeaktionen an
    void ProcessResponse(const Request& request, const Response& response)
    {
         // Anhand der Request-ID wird die enstsprechende Funktion geladen
         ProcessResponseFunc processResponse = m_processResponseFuncMap[request.GetId()];
         processResponse(request, response); // Antwort überprüfen und bearbeiten
    }
private:
    typedef std::map<int, ProcessResponseFuncPtr> ProcessResponseFuncMap;
    ProcessResponseFuncMap m_processResponseFuncMap;
};

Pro Request-Typ müsste eine Request und Response-Klasse erstellt werden sowie eine Funktion geschrieben und beim Client registriert werden, die den eigentlichen Abgleich und die Bearbeitung durchführt. Innerhalb dieser Funkion würden dann aber gleich zwei dynamic_cast (bzw. static_cast ) durchgeführt.

hustbaer schrieb:

* meinst du nicht vielleicht Response?

Ja. *ups*

[hustbaer]

Naja, klar. Performance-mässig ist es egal, da der ganze Netzwerk-Quargel 100x langsamer sein wird.
Indiz für schlechtes Design? Wieso? Du bekommst ja vom Deserializer etwas zurück dessen Typ du nicht voraussagen kannst - wie sollst du denn sonst prüfen ob es das ist was du erwartet hast?

Die einzige andere Möglichkeit (die mir jetzt einfällt) wäre irgendwelche eigenen Typ-IDs zu verwenden (oder welche die das Protokoll schon vorgibt). Nur dann müsste sich der Programmteil der deinen Client verwenden will mit etwas rumplagen was eigentlich nur den Deserializer zu interessieren braucht. Finde ich nicht gut.

Stimme ich Dir zu. Aber ich wollte, dass man nur eine Klasse hinzufügen muss, wenn man ein neuen Request-Typ hinzufügen möchte. Ich finde einfach keinen besseren Ansatz.

Ich hätte an sowas in der Art gedacht. Weiss natürlich nicht wie gut das zu dem passt was du vor hast...

// Basisklassen
class Request {}; 

class Response
{
public:
	virtual ~Response() {};
};

// Ein konkreter Request + seine Response
class FavoriteNumberResponse;

class FavoriteNumberRequest : public Request
{
public:
	typedef FavoriteNumberResponse ResponseType;

	FavoriteNumberRequest(unsigned int userId_)
		: userId(userId_) { }

	unsigned int userId;
};

class FavoriteNumberResponse : public Response
{
public:
	int favoriteNumber;
};

// Der Client

class Client
{
public:
	template <class REQ, class H>
	void SendRequest(REQ const& request, H handler)
	{
		QueueEntry<REQ::ResponseType, H>* entry = new QueueEntry<REQ::ResponseType, H>(handler);
		SendRequestImpl(request, entry);
	}

private:
	struct QueueEntryBase
	{
		virtual void HandleResponse(Response& response) = 0;

		int requestToken;
	};

	template <class RES, class H>
	struct QueueEntry : QueueEntryBase
	{
	public:
		explicit QueueEntry(H handler_)
			: handler(handler_) { }

		virtual void HandleResponse(Response& response)
		{
			handler(dynamic_cast<RES&>(response));
		}

		H handler;
	};

	void SendRequestImpl(Request const& request, QueueEntryBase* queueEntry)
	{
		std::auto_ptr<QueueEntryBase> guard(queueEntry);

		int const requestToken = SerializeAndSendRequest(request);
		queueEntry->requestToken = requestToken;
		Enqueue(queueEntry);
		// NOTE: Die elegantere Lösung wäre den Eintrag erstmal in die Queue
		// zu stecken und dann erst die Anfrage zu senden.
		// Dazu bräuchte man einen Guard der im Falle einer Exception
		// den Eintrag erstmal wieder aus der Queue entfernt und
		// dann erst löscht.

		guard.release();
	}

	void OnResponseReceived(int requestToken, Response& response)
	{
		std::auto_ptr<QueueEntryBase> guard(Dequeue(requestToken));
		if (guard.get())
			guard->HandleResponse(response);
		else
			throw std::runtime_error("WTF!?!");
	}

	int SerializeAndSendRequest(Request const& request);

	void Enqueue(QueueEntryBase* queueEntry);
	QueueEntryBase* Dequeue(int requestToken);

};

// Verwendung

void MyHandler(FavoriteNumberResponse r)
{
	std::cout << r.favoriteNumber;
}

int main()
{
	Client c;
	c.SendRequest(FavoriteNumberRequest(42), &MyHandler);

	// wait

	return 0;
}

Dadurch muss derjenige der den Client verwendet keine eigenen Klassen von irgendwas ableiten, nur damit er die Antwort verarbeiten kann.
Wenn du im Handler wirklich den originalen Request nochmal brauchst kannst du das entweder dem aufhalsen der den Client verwendet (er kann sich ja eine Kopie des Requests mit boost::bind() in seinen Handler reinbinden), oder das QueueEntry Template entsprechend anpassen.

[Roger Wilco]

Vielen Dank für Dein Vorschlag/Beispiel! Toll, dass du Dir die Mühe mit dem ausführlichen Code-Beispiel gemacht hast!

Das ganze hat mir sehr weitergeholfen. Besonders die Idee mit dem seperaten Handler finde ich gut.

Eine Frage nochmal zum dynamic_cast :

Ich habe ja die Option, durch eine Art Typ-ID, die im Protokoll definiert ist, den genauen Typ der Response festzustellen. Solange der Server keine fehlerhafte Antwort sendet, ist der genaue Response-Typ somit klar. Wäre dann evtl. ein Abgleich der Typ-ID + static_cast sinnvoller? Ein dynamic_cast würde eher vermuten lassen, dass es normal wäre, dass verschiedene Response-Typen eintreffen.

template <class RES, class H>
    struct QueueEntry : QueueEntryBase
    {
    public:
        explicit QueueEntry(H handler_)
            : handler(handler_) { }

        // Variante 1 mit dynamic_cast:
        virtual void HandleResponse(Response& response)
        {
            handler(dynamic_cast<RES&>(response)); //solange der Server keine fehlerhafte Antwort gesendet hat,
                                                   //MUSS response ein RES sein!
        }

        // Variante 2 mit static_cast:
        virtual void HandleResponse(Response& response)
        {   
            if(response.typeId != RES.typeId)
                throw runtime_error("Server hat fehlerhafte Antwort gesendet!");
            handler(ststic_cast<RES&>(response));
        }

        H handler;
    };

[ipsec]

Sinnvoll wäre ein static_cast , aber voher ein assert mit dynamic_cast . boost::polymorphic_downcast tut genau das, das könntest du also u.U. gleich verwenden

[hustbaer]

Hihi, mir fällt gerade auch dass DeserializeAndSendRequest in meinem Beispiel Blödsinn ist, es muss natürlich SerializeAndSendRequest heissen :). Werd ich gleich mal korrigieren.

@ipsec:
Da die Antwort hier von einem anderen Programm stammt, würde ich persönlich auf jeden Fall einen Check verwenden, der auch in der Release-Version noch drinnen ist.
Sonst könnte ein Fehler in einem Fremden Programm (dem Server) mein Programm dazu bringen zu crashen - oder schlimmer noch: nicht crashen aber falsche Ergebnisse liefern.

Und selbst wenn alles in einem Prozess läuft, würde ich in einigen Fällen den Release-Check vorziehen. Nämlich dann wenn die beiden Module "Client" und "Server" recht "weit" voneinander entfernt sind. z.B. wenn ein Programmierer/Team den Client macht und ein anderer Programmierer/anderes Team den Server.

Die paar Taktzyklen die ein dynamic_cast zusätzlich verplämpert kann man sich an der Stelle glaub' ich leisten

@Roger Wilco:
Ich weiss nicht was für ein Protokoll du da implementierst, daher hab' ich mal angenommen dass es u.U. verschiedenen Antwort-Typen auf den selben Request-Typ geben könnte.

Beispielsweise könnte es eine "basic" und eine "erweiterte" Antwort geben, je nachdem was die Daten in der Anfrage sind.

Um mein "favorite number" Beispiel wieder aufzugreifen... sagen wir es gibt einen "UserFavoritesRequest", und der Server darf dann - je nachdem was er für Daten für den bestimmten User hat - eine "BasicFavoritesResponse" mit nur der "favorite number" oder eine "ExtendedFavoritesResponse" mit "favorite number" und "favorite color" schicken.

In dem Fall müsste man dann wieder erlauben, dass der Typ nicht 1:1 mit dem erwarteten übereinstimmt.

Anderer Fall wo es nötig ist: wenn die Response-Klassen nur Interfaces sind, und es Aufgabe des Deserializers ist passende Implementierungen zu haben/erzeugen. Dann würde der dynamische Typ der Response nie mit Request::ResponseType übereinstimmen.

Wenn beides für dich kein Thema ist, dann kannst du natürlich getrost die "check typeid + static_cast" Variante nehmen.

[Roger Wilco]

@hustbaer: Den zweiten Fall müstest Du mir nochmal genauer erklären - den habe ich nicht verstanden.

Ansonsten ist es in meinem Fall so, dass zu jedem Request-Typ exakt ein Response-Typ definiert ist (Request.TypeId == Response.TypeId). Trotzdem muss ich davon ausgehen, dass ein Server ein Fehler macht und mir eine falsche Antwort schickt. Das wäre aber definitiv ein Bug im Server.

Einfaches Beispiel:

struct Request
{
    explicit Request(unsigned char id) : id(id) {}
    virtual ~Request {};
    unsigned char     id;
};

struct Response
{
    explicit Response(unsigned char id) : id(id) {}
    virtual ~Response {};
    unsigned char     id;
};

struct ReadRegisterRequest : public Request
{
    ReadRegisterRequest : Request(ID::READ_REGISTER) {}  
    uint16_t  startAddress;   // Startadresse
    uint16_t  count;          // Anzahl Register
};

struct ReadRegisterResponse : public Response
{
    ReadRegisterResponse : Response(ID::READ_REGISTER) {}
    uint16_t  startAddress;    // == Request.startAddress !
    uint16_t  count;           // == Request.count !
    std::vector<int16_t> data; // data.size() == count
};

[hustbaer]

Bei dem einfachen Beispiel macht es keinen Sinn, aber ich meine sowas:

struct Response
{
    explicit Response();
    virtual ~Response();
};

struct ReadRegisterResponse : public Response
{
    virtual uint16_t GetStartAddress() const = 0;
    virtual uint16_t GetCount() const = 0;
    virtual int16_t GetData(uint16_t offset) const = 0;
};