Bad Practise

Hey Leute ich hatte letztens ein Open Source Programm veröffentlicht allerdings wurde es oft mit Bad Practise in diesen Zeilen kommentiert.
Kann mir jemand helfen.

time_t t = time(0);
    struct tm * now = localtime(& t);
    std::ofstream log("server.log", std::ios::app);
    log << now->tm_hour << ":" << now->tm_min << ":" << now->tm_sec << " [INFO] " + message << "\n";
    std::cout << now->tm_hour << ":" << now->tm_min << ":" << now->tm_sec << " [INFO] " + message << "\n";

camper

wobei? Herausfinden, was mit bad practise gemeint ist? Oder wie verbessert werden sollte?

Skym0sh0

Mh, was mir auffällt und was ich vermeiden würde, wäre dieses C-artige Konstrukt mit localtime.

Aber mir fällt da auch keine Alternative ein grad oO

Bashar

Ich verstehe die Frage nicht. Das Programm wurde oft negativ kommentiert, aber die Kommentatoren haben sich alle verschworen und wollen dir nicht sagen, was genau nicht OK ist? Auch auf Nachfrage nicht?

Ich würde gerne wissen was ich verbessern könnte.

Bashar

Das + in den beiden Ausgaben durch << ersetzen wäre ein eindeutiger Kandidat.

void Write(struct tm* _tm, std::ofstream& os){
 os << _tm->tm_hour << .... << "\n"; }
Write(now, log);
Write(now, std::cout);

Ruvi

Skym0sh0 schrieb:

Mh, was mir auffällt und was ich vermeiden würde, wäre dieses C-artige Konstrukt mit localtime.

Aber mir fällt da auch keine Alternative ein grad oO

Boost hat wohl auch eine datetime Library aber ich habe selbst damit noch nichts gemacht.

Kellerautomat

Formatstrings waeren mal was, siehe https://www.c-plusplus.net/forum/p2437357#2437357.

TNA

struct tm * now = localtime(& t);

Dieser Code ist nicht unbedingt threadsafe, da localtime in gängigen Implementierungen einen Zeiger auf ein interne statisches struct zurückgibt, dass sich mit jedem Aufruf ändert. Der Standard liefert aber leider keine Alternative.

SeppJ

TNA schrieb:

struct tm * now = localtime(& t);
Dieser Code ist nicht unbedingt threadsafe, da localtime in gängigen Implementierungen einen Zeiger auf ein interne statisches struct zurückgibt, dass sich mit jedem Aufruf ändert. Der Standard liefert aber leider keine Alternative.

Man kann's halt direkt kopieren, was zumindest Folgeaufrufe ermöglicht. Oder, da es wohl um die aktuelle Uhrzeit geht, die bekommt man auch mit der system_clock aus chrono.

TNA

SeppJ schrieb:

Man kann's halt direkt kopieren, was zumindest Folgeaufrufe ermöglicht.

Da der Kopiervorgang nicht atomar ist, müsstest du sicherstellen, das während du die Kopie macht niemand anderes localtime aufruft. Das ist in größeren Projekten quasi unmöglich.

SeppJ schrieb:

Oder, da es wohl um die aktuelle Uhrzeit geht, die bekommt man auch mit der system_clock aus chrono.

Wenn du die so ausgeben wolltest müsstest du den time_point erst in eine time_t und davon in ein tm* umwandeln. Damit hast du die selben Probleme und schöner wird der Code dadurch auch nicht.

Torsten Robitzki

TNA schrieb:

struct tm * now = localtime(& t);
Dieser Code ist nicht unbedingt threadsafe, da localtime in gängigen Implementierungen einen Zeiger auf ein interne statisches struct zurückgibt, dass sich mit jedem Aufruf ändert. Der Standard liefert aber leider keine Alternative.

Plattformen, die threads unterstützen legen diese Werte typischer Weise in thread local storage ab.

Torsten Robitzki

odu schrieb:

Hey Leute ich hatte letztens ein Open Source Programm veröffentlicht allerdings wurde es oft mit Bad Practise in diesen Zeilen kommentiert.
Kann mir jemand helfen.

Du öffnest mit jeder Zeile die Datei? Das ist Gift für die Performance.

Warum schreibst Du nicht einfach auf std::clog und überlässt es dem Aufrufer, den output entsprechend dahin zu biegen, wo er ihn haben möchte?

mfg Torsten

Torsten Robitzki

großbuchstaben schrieb:

void Write(struct tm* _tm, std::ostream& os){
 os << _tm->tm_hour << .... << "\n"; }

Die Funktion nehme ich und dann schreibe ich dazu noch:

std::ostream& now( std::ostream& o )
{
  ...
  Write( localtime(&t), o );
  return o;
}

und dann wird's hübscher :

std::clog << now << message << std::endl;

Edit: stream / ofstream => ostream

Torsten Robitzki schrieb:

großbuchstaben schrieb:
void Write(struct tm* _tm, std::ofstream& os){
 os << _tm->tm_hour << .... << "\n"; }
Die Funktion nehme ich und dann schreibe ich dazu noch:
std::ostream& now( std::stream& o )
{
  ...
  Write( localtime(&t), o );
  return o;
}
und dann wird's hübscher :
std::clog << now << message << std::endl;

Diesen Code versteh ich jetzt nicht.

Wie wird der Text ausgegeben?

Torsten Robitzki

odu schrieb:

Diesen Code versteh ich jetzt nicht.

Wie wird der Text ausgegeben?

std::clog << message << std::endl;

Den Teil verstehst Du doch soweit oder? Die Write() Funktion von Großbuchstaben verstehst Du auch, oder?

std::ostream (der Typ, den std::clog hat), hat eine Funktion (operator<<), die als Parameter eine Funktion nimmt, die einen ostream& nimmt. Die sieht in etwa so aus:

std::ostream& operator<<(std::ostream (*f)(std::ostream&))
{
   return f( *this );
}

*this ist im obigen Beispiel std::clog und f ist die Funktion now(). Der obige operator<< ruft also now() mit *this auf. Und now() ruft Write() auf. Das ist übrigens der gleiche Mechanismus, mit dem std::endl einen "\n" in den stream bekommt und anschließend flush() aufruft.

mfg Torsten

doch der std::stream& o wurde nirgendswo defeniert

Kann mir wer klären, was die Zeile

struct tm * now = localtime(& t);

bedeutet?
Das tm * now bringt mich ins grübeln.

SeppJ

struct tm ist der Name des Typs in der C-Schreibweise, wo man mit angeben muss, dass es ein struct ist. In C++ könnte man auch bloß tm schreiben, aber die C-Schreibweise geht eben auch. Nehmen wir mal zur Vereinfachung an, da stünde einfach nur int als Typ:

int * now = irgendwas;

Nun klar? Falls nicht: Leerzeichen sind egal. Das ist das gleiche wie

int *now = irgendwas;

oder

int* now = irgendwas;

oder

int       *      now = irgendwas;

Also bloß ein Zeiger namens now auf einen Typen namens struct tm .