Status-Klasse auslagern



  • Hallo,

    ich bin dazu übergegangen, bei großen Klassen die Members, die sich auf den Status eines Objekts der Klasse beziehen, als eigene Klasse auszulagern, um so ein Fat Interface zu vermeiden.

    Dabei verwende ich zwei Status-Klassen, RStatus und RWStatus, wobei RStatus public ist, aber nur das Lesen von Status-Eigenschaften erlaubt, und RWStatus private ist, aus RStatus abgeleitet ist und das Ändern von Status-Eigenschaften erlaubt.

    Das sieht dann z. B. so aus ( SHP ist ein Makro für boost::shared_pointer ):

    /// Reversible node which stores site-wise nucleotide probabilities.
    class Prob_Nd : public virtual Rev_Base_Nd
    public:
    	class RStatus;
    	SHP<RStatus> r_status() const { return static_pointer_cast<RStatus>(m_status); }   ///< Get the status of this node.
    
    	...
    
    private:
    	class RWStatus;
    	SHP<RWStatus> rw_status() const { return m_status; }   ///< Get the status of this node.
    	SHP<RWStatus> m_status;   ///< status of this node
    
    	...
    };
    
    /// %Status class of Prob_Nd, allowing only to read status.
    class Prob_Nd::RStatus
    {
    public:
    	/** @name Construction & Destruction */
    	//@{
    	RStatus(Prob_Nd* nd) : m_nd(nd) { }   ///< Constructor.
    	//@}
    
    	/** @name Getter */
    	/// Get the node this object is associated to.
    	Prob_Nd* nd() const { return m_nd; }
    	/// Have the LSPs been initialized?
    	BOOL are_LSPs_alloc() const { return m_are_LSPs_alloc; }
    	/// Had the LNPs of this node to be recalculated due to tree changes?
    	BOOL are_own_LNPs_aff() const { return m_are_own_LNPs_aff; }
    	/// Had the LNPs for mutation to ancestor to be recalculated due to tree changes?
    	BOOL are_anc_LNPs_aff() const { return m_are_anc_LNPs_aff; }
    	/// Are the LSPs up to date?
    	BOOL are_anc_LSPs_UTD() const { return m_are_anc_LSPs_UTD; }
    	//@}
    protected:
    	/** @name Setter */
    	void mark_LSPs_alloc() { m_are_LSPs_alloc = true; }
    	/// Mark this node that it has had its own LNPs recalculated due to changes to the tree.
    	void mark_own_LNPs_aff() { m_are_own_LNPs_aff = true; }
    	/// Remove mark that this node has had its own LNPs recalculated due to changes to the tree.
    	void mark_own_LNPs_unaff() { m_are_own_LNPs_aff = false; }
    	/// Mark this node that it had its LNPs for mutation to its ancestor(s) recalculated due to changes to the tree.
    	void mark_anc_LNPs_aff() { m_are_anc_LNPs_aff = true; }
    	/// Remove mark that this node has had the LNPs for mutation to its ancestor(s) recalculated due to changes to the tree.
    	void mark_anc_LNPs_unaff() { m_are_anc_LNPs_aff = false; }
    	/// Mark this node that its LSPs are up to date
    	void mark_anc_LSPs_UTD() { m_are_anc_LSPs_UTD = true; }
    	/// Mark this node that its LSPs are outdated
    	void mark_anc_LSPs_outdated() { m_are_anc_LSPs_UTD = false; }
    	//@}
    private:
    	Prob_Nd* m_nd;   ///< The node this object is associated to
    	BOOL m_are_LSPs_alloc;   ///< Have the LSPs been initialized?
    	BOOL m_are_own_LNPs_aff;   ///< have the LNPs of this node to be recalculated due to tree changes?
    	BOOL m_are_anc_LNPs_aff;   ///< have the LNPs for mutation to ancestor to be recalculated due to tree changes?
    	BOOL m_are_anc_LSPs_UTD;   ///< have the substitution probabilities for mutation to ancestor to be recalculated due to tree changes?
    };
    
    /// %Status class of Prob_Nd, allowing for both reading and writing of the status.
    class Prob_Nd::RWStatus : public Prob_Nd::RStatus
    {
    public:
    	RWStatus(Prob_Nd* nd) : RStatus(nd) { }   ///< Constructor.
    
    	using RStatus::mark_LSPs_alloc;
    	using RStatus::mark_own_LNPs_aff;
    	using RStatus::mark_own_LNPs_unaff;
    	using RStatus::mark_anc_LNPs_aff;
    	using RStatus::mark_anc_LNPs_unaff;
    	using RStatus::mark_anc_LSPs_UTD;
    	using RStatus::mark_anc_LSPs_outdated;
    };
    

    Ich frage mich nun, ob das so die beste Lösung ist oder ob das irgendwie einfacher geht. Dabei möchte ich auf jeden Fall haben, dass nicht von überall einfach Status-Eigenschaften geändert werden können.



  • Was genau ändert sich jetzt, ausser dass du viel mehr tippen musst, ein seltsames Idiom verwendest das keiner kennt und haufenweise unnötige shared_ptr Kopien machst?
    Letzteres wird dir u.U. so richtig "schön" bei der Performance reinhauen - vor allem wenn du wirklich selbst innerhalb der Klasse über die (mMn. total sinnfreie) rw_status() Funktion gehst.

    Das Fat Interface hast du doch immer noch, es heisst jetzt bloss Prob_Nd::RStatus statt Prob_Nd .

    Also ich sehe den Sinn dahinter nicht.

    Ein Fat Interface vermeidet man indem man das Design ändert. Also die "responsibilities" anders auf Klassen verteilt.
    Bzw. manchmal eben gar nicht. Es gibt einfach Fälle wo es schwer bis unmöglich ist alles in schön kleine überschaubare Happen zu zerteilen.

    Wie das in deinem Fall aussieht kann ich nicht sagen. Statistische Analysen von DNA-Gedöns (richtig geraten?) sind so überhaupt nicht meine Domäne, von daher sind das alles böhmische Dörfer für mich.
    Funktionsbeschreibungen wie "are the LSPs up to date?" klingen aber danach, als ob es hier u.U. ein Problem mit der Kapselung oder Aufgabenverteilung geben könnte.



  • Und tatsächlich ein Fall von zu viel Dokumentation brrrrrr, ich kann da erstmal gar nichts erkennen



  • Nur eine Kleinigkeit, aber warum verwendest du BOOL statt den eingebauten C++ typ bool?



  • Ohne die unnötige Redundanz kann das so aussehen:

    /// %Status of Prob_Nd
    class Status
    {
    public:
        bool are_LSPs_alloc;   ///< Have the LSPs been initialized?
        bool are_own_LNPs_aff;   ///< have the LNPs of this node to be recalculated due to tree changes?
        bool are_anc_LNPs_aff;   ///< have the LNPs for mutation to ancestor to be recalculated due to tree changes?
        bool are_anc_LSPs_UTD;   ///< have the substitution probabilities for mutation to ancestor to be recalculated due to tree changes?
    
        Status()
    		: are_LSPs_alloc(false)
    		, are_own_LNPs_aff(false)
    		, are_anc_LNPs_aff(false)
    		, are_anc_LSPs_UTD(false)
    	{ }
    
    };
    
    /// Reversible node which stores site-wise nucleotide probabilities.
    class Prob_Nd : public virtual Rev_Base_Nd
    public:
        Status status() const { return m_status; }
    
        ...
    
    private:
    
        Status m_status;
    
        ...
    };
    


  • hustbaer schrieb:

    Was genau ändert sich jetzt, ausser dass du viel mehr tippen musst, ein seltsames Idiom verwendest das keiner kennt und haufenweise unnötige shared_ptr Kopien machst?
    Letzteres wird dir u.U. so richtig "schön" bei der Performance reinhauen - vor allem wenn du wirklich selbst innerhalb der Klasse über die (mMn. total sinnfreie) rw_status() Funktion gehst.

    Das Fat Interface hast du doch immer noch, es heisst jetzt bloss Prob_Nd::RStatus statt Prob_Nd .

    Also ich sehe den Sinn dahinter nicht.

    Ein Fat Interface vermeidet man indem man das Design ändert. Also die "responsibilities" anders auf Klassen verteilt.
    Bzw. manchmal eben gar nicht. Es gibt einfach Fälle wo es schwer bis unmöglich ist alles in schön kleine überschaubare Happen zu zerteilen.

    Wie das in deinem Fall aussieht kann ich nicht sagen. Statistische Analysen von DNA-Gedöns (richtig geraten?) sind so überhaupt nicht meine Domäne, von daher sind das alles böhmische Dörfer für mich.
    Funktionsbeschreibungen wie "are the LSPs up to date?" klingen aber danach, als ob es hier u.U. ein Problem mit der Kapselung oder Aufgabenverteilung geben könnte.

    Die Intention war, den Umfang des Interfaces von Prob_Nd zu reduzieren. Die Doxygen-Dokumentation von Prob_Nd ging über 3 Seiten (hochgestellter 24-Zoller) und das war mir zu unübersichtlich. Da der Status einer Klasse eine sauber abgegrenzte Entität ist und auch nicht zu den Kernaufgaben der Klasse gehört, hatte ich mich entschieden, ihn auszulagern. Also anstatt den Status als Data members zu repräsentieren, Composite Aggregation zu verwenden.

    Performance wird hier keine Rolle spielen. Der Status wird lediglich abgefragt, um beim Aufruf von bestimmten Methoden sicherzustellen, dass die Instanz von Prob_Nd den Aufruf der Methode erlaubt bzw. was die Methode machen soll.

    Wenn ich eine Möglichkeit sehen würde, die Responsibilities von Prob_Nd einfach zu trennen, würde ich das natürlich machen. Die Einführung der Status-Klassen war der Versuch, die Übersichtlichkeit des Codes zu erhöhen im Angesicht dessen, dass mir das Auftrennen der Responsibilities nicht gelingt.

    Es geht bei dem Projekt um phylogenetische Analyse von rekombinanten Viren, also Bioinformatik. Dabei muss man eine bestimmte Klasse von Graphen modifizieren und anschließend die Likelihood des Graphen berechnen. Wenn die LSPs einer Prob_Nd-Instanz nicht mehr up-to-date sind, bedeutet dies, dass der Graph so modifiziert wurde, dass die LSPs der Instanz im Rahmen der Berechnung der Likelihood neu berechnet werden müssen.

    Falls es jemand genau wissen will: Hier die Publikation, in der der Vorläufer des Tools, an dem ich gerade arbeite, beschrieben ist: http://www.ncbi.nlm.nih.gov/pubmed/?term=20400454

    Namenloser324 schrieb:

    Und tatsächlich ein Fall von zu viel Dokumentation brrrrrr, ich kann da erstmal gar nichts erkennen

    Ich hielt es für aussichtslos, den Code hier so zu präsentieren, dass man basierend auf den Kommentaren verstehen kann, was auf fachlicher Ebene passiert. Dafür müsste man dann in die Doxygen-Doku und das Developer Manual schauen, was aber für eine Frage in einem Forum offensichtlich Overkill wäre.

    Scorcher24 schrieb:

    Nur eine Kleinigkeit, aber warum verwendest du BOOL statt den eingebauten C++ typ bool?

    BOOL hatte ich eingeführt, da ich eine Range-checked Vektor verwende, der mit bool nicht klar kam. Das ist ziemlich sicher nicht die optimale Lösung, war aber damals das, was mir eingefallen war, und ist für meine Zwecke erst mal ok.



  • ingobulla schrieb:

    Die Intention war, den Umfang des Interfaces von Prob_Nd zu reduzieren. Die Doxygen-Dokumentation von Prob_Nd ging über 3 Seiten (hochgestellter 24-Zoller) und das war mir zu unübersichtlich. Da der Status einer Klasse eine sauber abgegrenzte Entität ist und auch nicht zu den Kernaufgaben der Klasse gehört, hatte ich mich entschieden, ihn auszulagern. Also anstatt den Status als Data members zu repräsentieren, Composite Aggregation zu verwenden.

    Dann mach es so wie TyRoXx vorgeschlagen hat.


Log in to reply