Designfrage: dynamic_cast hier angebracht?

[Roger Wilco]

Hmm, da muss ich mich mal einlesen, da ich das Muster nicht kenne.

Ich habe hier noch ein Thread mit einem sehr ähnlichen Fall von Tachyon gefunden. So wie ich das verstehe, bleibt Tachyon da beim dynamic_cast :

Erweitertes Kommandomuster und ekliger dynamic_cast...

In meinem Fall habe ich aber ein Typ-Identifizierer in der Basisklasse Response , da die Protokoll-Spezifikation eine ID für jedes Request/Response-Paar vorsieht. Damit wäre doch folgende Variante möglich und sinnvoller, oder?

//RequestA.h:

#include Request.h

class RequestA : public Request //ein Auftragstyp
{
   ...
   void ProcessResponse(Response* response)
   {
       if(response.GetId() != m_id) throw WrongResponseException();

       ResponseA* responseA = static_cast<ResponseA*>(response);
       // Antwort auswerten ...
   }
   ...
};

[dot]

Natürlich geht das auch. Damit hast du dir dann effektiv einfach selber ein typeid gebaut...

Mal ne grundsätzliche Frage: Du sagst du hast ein Protokoll in dem es zu jedem Request eine genau definierte Respone gibt. Inwiefern ist die Aufteilung von Request und Reponse in zwei getrennte Klassen hier notwendig (darin würde ich nämlich erst die Ursache all deiner Probleme sehen, darum wärs vielleicht interessant mal kurz zu überlegen was genau du eigentlich davon hast)? Wenn es einfach eine Reihe von Requests gibt und zu jedem Request genau eine mögliche Response, warum können Request und Response dann nicht in eine Klasse die sich um beides (senden des Request und parsen der Response) kümmert? Und wenn wir dann schon dabei sind: brauchst du überhaupt die Laufzeitpolymorphie? Ein Downcast (egal ob dynamic oder nicht) ist imo jedenfalls praktisch immer Symptom für einen Designfehler.

[Roger Wilco]

dot schrieb:

Natürlich geht das auch. Damit hast du dir dann effektiv einfach selber ein typeid gebaut...

Wäre die Frage, ob es dem dynamic_cast vorzuziehen wäre?

dot schrieb:

Wenn es einfach eine Reihe von Requests gibt und zu jedem Request genau eine mögliche Response, warum können Request und Response dann nicht in eine Klasse die sich um beides (senden des Request und parsen der Response) kümmert?

Ja, das habe ich auch überlegt. Es ist tatsächlich so, dass der Empfänger des Requests (Server) einfach den Header (inkl Type-ID) kopiert und dann je nach Request ein ACK oder die gewünschten Daten anhängt. Der Empfänger des Response muss nur überprüfen, ob die Header identisch sind und dann die Antwort auswerten. Nur wenn der Server ein Fehler melden möchte, gibt es eine Exception als Antwort. Die ist aber für alle Requests gleich.
Wichtig ist auch, dass (im Gegensatz zu dem Problem von Tachyon) bei mir die Zuordnung von Response zu Request sicher gestellt ist, indem es entweder synchron abläuft (Request1-Response1-Request2-Response2-...) oder beim asynchronen Betrieb es eine zusätzliche Schicht gibt, die die Zuordnung sicher stellt.

Das dynamic_cast ist bei mir ein "Hat der Server nichts falsch gemacht?" und kein "Ist die Antwort für mich gedacht?".

dot schrieb:

Und wenn wir dann schon dabei sind: brauchst du überhaupt die Laufzeitpolymorphie?

Denkst Du an eine Lösung mittels Templates? Ansich benötige ich die Polymorphie nur, da ich zunächst ein Teil in der Spezifikation definierten Requests umsetzten möchte, da wir nur einen Teil benötigen. Das soll aber später erweiterbar sein.

dot schrieb:

Ein Downcast (egal ob dynamic oder nicht) ist imo jedenfalls praktisch immer Symptom für einen Designfehler.

Das dachte ich mir auch und deswegen bin ich froh, dass Anfängern wie mir hier etwas auf die Sprünge geholfen wird.

[dot]

Roger Wilco schrieb:

dot schrieb:

Natürlich geht das auch. Damit hast du dir dann effektiv einfach selber ein typeid gebaut...

Wäre die Frage, ob es dem dynamic_cast vorzuziehen wäre?

Das hängt davon ab, generell sind alle 3 Möglichkeiten zu vermeiden wenn du mich fragst. typeid bzw. die selbst gebaute ID haben evtl. einen kleinen Performancevorteil gegenüber dynamic_cast. Wobei die Performance hier sowieso keine Rolle spielen sollte.

Roger Wilco schrieb:

Es ist tatsächlich so, dass der Empfänger des Requests (Server) einfach den Header (inkl Type-ID) kopiert und dann je nach Request ein ACK oder die gewünschten Daten anhängt. Der Empfänger des Response muss nur überprüfen, ob die Header identisch sind und dann die Antwort auswerten. Nur wenn der Server ein Fehler melden möchte, gibt es eine Exception als Antwort. Die ist aber für alle Requests gleich.
Wichtig ist auch, dass (im Gegensatz zu dem Problem von Tachyon) bei mir die Zuordnung von Response zu Request sicher gestellt ist, indem es entweder synchron abläuft (Request1-Response1-Request2-Response2-...) oder beim asynchronen Betrieb es eine zusätzliche Schicht gibt, die die Zuordnung sicher stellt.

Das dynamic_cast ist bei mir ein "Hat der Server nichts falsch gemacht?" und kein "Ist die Antwort für mich gedacht?".

Wenn es zu jedem Request nur eine Mögliche Antwort gibt seh ich keinen Grund für diese Aufteilung in zwei Klassen. Das ganze klingt für mich eher so als wärs besser Request und Response in einer Klasse zu handeln. Du kannst ja Hilfsfunktionen schreiben die das Vergleichen der Header und testen ob es eine "Exception" ist übernehmen oder das ganze evtl. in eine Basisklasse stecken. Der Punkt ist: Die Klasse parsed einfach nurmehr die Daten die zurückkommen und fertig, ich seh im Moment zumindest absolut keinen Grund warum man einem Request alle möglichen Responses übergeben können sollte. Wenn du mich fragst ist das ein Designfehler (und der Grund für all deine Probleme) da dein Interface damit etwas erlaubt was eigentlich gar nicht erlaubt sein sollte (jede Art von Request kennt nur eine mögliche Response, warum sollte man ihr also jede mögliche Response übergeben können?).

Roger Wilco schrieb:

Denkst Du an eine Lösung mittels Templates? Ansich benötige ich die Polymorphie nur, da ich zunächst ein Teil in der Spezifikation definierten Requests umsetzten möchte, da wir nur einen Teil benötigen. Das soll aber später erweiterbar sein.

Nein eigentlich denk ich nur dran ob du wirklich diese Basisklassen und virtuellen Funktionen brauchst oder obs eine einfache RequestA, RequestB, RequestC Klasse nicht auch tun würde. Warum z.B. nicht einfach so:

class RequestA
{
  ... 
  void ProcessResponse(const ResponseA& response)
  {
    // handle response
  }
  ... 
};

[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;
};