3. Codedesign: Logging Class

Codedesign: Logging Class

  • M

    Hi,

    ich denke ich habe mich in den letzten Wochen gut in C++ eingearbeitet. Was mir jetzt noch fehlt, sind die "Erfahrungswerte" in Bezug auf gutes Codedesign.

    Nocheinmal am Beispiel einer Logging-Klasse:

    1ter Vorschlag:

    // am Anfang
int main() {
  Log::open( "name.log" );
  ...
}

// in einer x-beliebigen Funktion
Log::write( "dies wird geloggt" );

    wobei open und write beides statische Methoden sind.

    2ter Vorschlag:

    // am Anfang
int main() {

  Log *instance = Log::getInstance();

  instance->open( "name.log" );
  ...
}

// in einer x-beliebigen Funktion
Log *instance = Log::getInstance();

instance->write( "dies wird geloggt" );

//oder auch möglich:
instance << "dies könnte auch geloggt werden";

    hier ist Log eine Singleton Klasse, bei der eine Instanz über die statische Methode getInstance geholt werden kann.

    3ter Vorschlag:

    // Global
Log log;

int main() {

  log.open( "name.log" );
  ...
}

// in einer x-beliebigen Funktion

log << "dies wird geloggt";

    Log ist ein globales Objekt. Mit << überladen.

    Wie gesagt, das sind alles Vorschläge, die durchgekaut habe.
    Vom Aufruf gefällt mir Nr 3 am Besten. Doch stört mich das globale Object.
    Habe mir irgendwann mal im Hinterkopf gespeichert, dass globale Objekte böse sind 🙄

    Was meint ihr?

    Grüße
    euer Maddes

    Ps: Die Codeschnipsel verstehen sich als stark gekürzte Beispiele 😉

    0
  • *

    1. Vorschlag:
    Und wie willst du dann den Filepointer beibehalten? Also das wird dir bei statischen Funktion schwer fallen, da die ja nicht ohneweiteres auf den this-Pointer zugreifen können.

    2. Vorschlag:
    Das ist schon einmal eine relativ gute Idee. So musst du dich nicht darum kümmern, wie du einen Log-Pointer weitergibst, damit du in allen Klassen damit arbeiten kannst, damit du globale Variablen vermeidest. Dein Singleton ist aber nicht so dolle implementiert. Wenn du den << operator bei der Klasse aber noch überlädst, wäre die Lösung die beste.

    3. Vorschlag
    Globale Variablen sind nicht gut. Kommst da schöner mit dem 2. Vorschlag und meinen Verbesserungsvorschlägen hin.

    0
  • M

    Hi,

    Und wie willst du dann den Filepointer beibehalten?

    Spontan würde ich so antworten:

    class Log {

  private:

    static ofstream log;

  public:

    static void write( ... ) {

      log << ...

  (...)

    Dein Singleton ist aber nicht so dolle implementiert

    Hilft mir 0 weiter.

    Wenn du den << operator bei der Klasse aber noch überlädst

    siehe Beispiel 😉

    Weitere Vorschläge?

    0
  • V

    Ich würde 2 und 3 kombinieren. Also die Loggingklasse zu einem Singleton machen und eine Instanz erszeugen. Die Klasse und die Instanz aber in einen eigenen Namespace packen. So ähnlich wie cout etc. Und auch eine Methode anbieten, um das Loggingfile zu ändern. Das dürfte für den Anwender die einfachste Variante sein.

    Achso zum Singleton: guck dir mal Loki an.

    0
  • M

    Die Klasse und die Instanz aber in einen eigenen Namespace packen

    Das ist eine Idee, die ich noch gar nicht in Betracht gezogen habe. Klassisches Wald/Baum Problem 🙂

    Werd da mal ein bisschen rumprobieren 😉

    Danke dir vielmals!!!

    Maddes

    0
  • *

    Hmm halt nochn Streamoperator überladen ... aber ansonnsten so:

    #if !defined(LOG_H__INCLUDED)
#define LOG_H__INCLUDED

class Log
{
private:
	Log();
	~Log();

public:
	static Log& instance() { static Log log; return log; }

public:
	bool	open(const std::wstring&);

private:
	std::wofstream m_stream;
};

#endif // LOG_H__INCLUDED

    #include "log.h"

Log::~Log()
{
	if (m_stream.is_open() == true)
		m_stream.close();
}

bool Log::open(const std::wstring& filename)
{
	if (m_stream.is_open() == true)
		m_stream.close();

	m_stream.open(filename.c_str(), std::ios::out);

	return (!m_stream ? false : true);
}

    #include "log.h"

int main()
{
	Log::instance().open(L"test.log");
	Log::instance() << "Test 1";
}

    Das wäre was nettes 😉

    0
  • M

    @(D)Evil

    starke Sache 👍

    sieht wirklich gut aus 🙂

    Mal ein Beispiel mit der Idee von viande mit nem Namensraum (Klasse Logging wie oben):

    namespace Log {

	Logging& log = Logging::getInstance();
}

    Überladen:

    void Logging::operator <<( char* msg ) 
{
	// hier in datei schreiben

}

    würde das schon genügen?

    Funktionieren tut es auf jedenfall, aber vielleicht habe ich ja noch was nicht bedacht 😕

    int main()
{
  using namespace Log;

  log << "moin moin";

}
    0
  • V

    Maddes schrieb:

    namespace Log {

    Logging log = Logging::getInstance();
    //Referenzzeichen weg
}

    Überladen:

    void Logging::operator <<(const char* msg )
//das const ist wichtig, sonst wird dein Aufruf unten fehlschlagen
{
    // hier in datei schreiben

}

    würde das schon genügen?

    Funktionieren tut es auf jedenfall, aber vielleicht habe ich ja noch was nicht bedacht 😕

    int main()
{
  using namespace Log;

  log << "moin moin";

}

    Echt, das hat funktioniert? :o
    Aber sonst sieht das von der Idee her so aus, ja. Vielleicht sogar lieber nur using Log::log.

    0
  • M

    Echt, das hat funktioniert? :o

    jaja, du weisst doch was gemeint war 😃
    Passiert, wenn man an zwei Codes gleichzeitig schreibt (ich glaub das können nur Frauen...)

    Jetzt noch eine Lösung zu folgenem und ich bin glücklich 🙂

    void Logging::operator <<( string msg ) 
// oder const char* msg oder wie auch immer man möchte

    funzt super:

    log << "blabla";

    aber mittels:

    void Logging::operator <<( string msg ) 
...
void Logging::operator ()( int loglevel )
...

    funktioniert sowas:

    log(1) << "blabla";

    leider nicht:

    Fehler: void-Wert nicht ignoriert wie es sein sollte

    Dachte mir, damit könnte man einen "Loglevel" (debug, warning, error, etc) mitschicken.

    Auf jedenfall schon mal ein dickes Danke 👍

    0
  • K

    Maddes schrieb:

    void Logging::operator <<( string msg ) 
// oder const char* msg oder wie auch immer man möchte

    funzt super:

    log << "blabla";

    Ja. Aber das hier funzt gar nicht super:

    int my_value = 42;
log << "Wert = " << my_value;

    Also: 'op<<' um zwei Dinge erweitern:

    1. Parameter sollte ein Template sein
    2. Zum Verketten muss der Log zurückgegeben werden.

    template <typename T>
Logging& Logging::operator <<(T const& msg)
{
    // msg in die Datei schreiben.
    return *this;
}

    /EDIT: Und für Deinen 'op()' gilt übrigens dasselbe.

    0
  • Y

    Maddes schrieb:

    Echt, das hat funktioniert? :o

    void Logging::operator <<( string msg ) 
...
Logging &Logging::operator ()( int loglevel ) //referenz auf *this zurückgeben, dann sollte das weiter unten funktionieren
...

    funktioniert sowas:

    log(1) << "blabla";

    Du musst IMHO einfach eine Referenz auf *this vom operator() zurückgeben, dann sollte das mit dem Loglevel funktionieren. (Hab da mal nicht auf const-correctness geachtet ⚠ )

    0
  • M

    Zum Verketten muss der Log zurückgegeben werden

    einfach eine Referenz auf *this vom operator() zurückgeben

    Danke euch beiden, das wars 🙂

    Die Template-Idee ist jetzt die Luxusklasse 👍

    so gefällt mir das!

    Danke an Alle!

    0
  • *

    Achja ... wäre vllt ganz gut wenn du noch mit templates arbeiten würdest, um für alle dateitypen den streamoperator nutzen zu können 😉

    0
  • V

    trotzdem geht an makros kein weg vorbei, wenn du vermeiden willst, daß die argumente (manchmal teure funktionsaufrufe) ausgewertet werden, wenn das eingestellte loglevel gerade sagt, daß diese zeile nicht loggen soll.

    0
  • M

    trotzdem geht an makros kein weg vorbei

    hmmf, ja, das durft ich grad selbst herausfinden 😞

    wenn ich zum Beispiel

    log(1) << "foo " << "bar";

    habe, schreibt er:

    DEBUG: hallo DEBUG: bar

    das ist natürlich auch Quatsch. Dort müsste man wahrscheinlich eine Flag setzten, wenn DEBUG (oder was auch immer) ausgegeben wurde, und bei einem Newline diese wieder löschen. Oder so in der Art 🙄

    Da mir das aber alles noch zu unklar ist, lasse ich dieses Feature lieber ersteinmal raus.

    Es ist da aber gleich noch ein Problem aufgetreten - und zwar mit dem Namesraum: (sorry für die noob-hafte Formulierung)

    Beim Testen habe ich also auf eine Header verzichtet, und alles in eine test.cpp gepackt:

    class Logging {
...
}

namespace Log {

  Logging log = Logging::getInstance();

}

int main() {

  Log::log << "hallo";

}

    das hat so (in der Art) auch funktioniert.

    Jetzt möchte ich aber die Implementierung und die Methodendefinition aber trennen, was nicht ganz klappt.

    Zum Beispiel:

    //class.Log.h

class Logging {
...
};

namespace Log {

  Logging log = Logging::getInstance();

}

    //class.Log.cpp

#include "class.Log.h"

Logging:: ...

    das will nicht recht. Auch habe ich versucht, die Klasse mit in den Namesraum zu packen.
    Fehler beim Kompilieren von test.cpp : multiple definition of `Log::log'

    Ich habs dann ersteinmal mit einem Makro gelöst:

    #define log Logging::getInstance()

    Aber weiss nicht, ob das so die geschickteste und sauberste Lösung ist 😕

    0
  • V
     

    //log.hpp
namespace Log
{
    class Logging
    {
        //...
    };

Logging log = Logging::getInstance();
}

//log.cpp
#include log.hpp
namespace Log
{
    Logging::Logging()
    {
    }
}

    Das geht nicht?

    0
  • *

    Der Sinn von dem ganzen Code ist nur fragwürdig, so wie ihr das im Moment macht ... dann kann man auch direkt globale Instanzen nehmen ...

    0
  • V

    (D)Evil schrieb:

    Der Sinn von dem ganzen Code ist nur fragwürdig, so wie ihr das im Moment macht ... dann kann man auch direkt globale Instanzen nehmen ...

    Klar kann man das, aber möchtest du gerne, dass an zwei verschiedenen Stellen geloggt wird, nur weil jemand auf die Idee kommt noch eine Instanz zu erzeugen?

    Alternativ wäre es auch möglich, alle Methoden und Attribute static zu machen. Find ich aber persönlich hässlich.
    Bleibt noch die Möglichkeit den Konstruktor privat zu machen und eine Friend-Methode statt der getInstanze()-Methode bereit zu stellen, aber da ist die Singleton-Variante schöner.
    Oder hast du noch einen Vorschlag?

    Außerdem wird die Instanz im Namespace "versteckt" und sollte daher für keine Konflikte sorgen. Und es erspart einem Tipparbeit und sorgt für Übersichtlichkeit. Find ich zumindest 🙂

    0
  • *

    Hmm du weißt das ich den Code zum Thema Singleton gepostet hab? Und ich mich mich eigentlich mit der globalen Instanz darauf bezog, das ihr hingeht, und, zwar in einem namespace, wieder eine Instanz habt. Das ist unnötig ... warum ruft ihr nicht einfach instance immer auf? Ist nun wirklich nicht soo viel aufwand.

    0
  • V

    Ich wollte dich nicht angreifen, tut mir Leid, wenn es vielleicht so rübergekommen ist.

    Worum es mir geht, ist, dass ich finde, dass die Variante mit getInstance einfach schwerer zu lesen ist. Außerdem sieht man nicht sofort, was da passiert und der Anwender der Loggingklasse muss sich darüber Gedanken machen, dass die Klasse als Singleton implementiert ist.

    Ich wollte einfach nur wissen, ob du vielleicht noch eine gute Lösung für das Problem hast. Würde mich auch im Allgemeinen mal interessieren.

    Logging::getInstance() << "Text";
//Hier müste ich erstmal raten, dass die Klasse vielleicht ein Singleton ist
//oder ich müsste in die Dokumentation gucken

logging::log << "Text";
//bzw
using logging::log;
log << "Text";
//Hier sieht man einfach auf einen Schlag, was passiert.
    0
Anmelden zum Antworten