Quelltextkommentare (Variablen, Funktionen etc.) fehlen noch allerorts. Wäre nett, hier zumindest die wichtigsten Annahmen zu beschreiben.
Auch innerhalb der neuen Algorithmen werden die grundlegenden Schritte nicht dokumentiert. Das macht es späteren Entwicklern schwerer, hier schnell mal Änderungen zu machen, ohne was kaputt zu machen. Bitte kommentiert die Algorithmen und verwendeten Datenstrukturen bitte wenigstens grundlegend ("was mache ich in welcher Reihenfolge?").
Bei Dokumentation im Header die Doxygen-Doku verwenden,
statt
/* used to get correct color of entity */
so schreiben, den Unterschied sieht man auch im QtCreator
/*! Used to get correct color of entity */
Wichtige wäre es, die Rückgabewerte/Fehlerbehandlung zu dokumentieren, bspw.
/*! Helper function to assign the correct block to an entity */
Block* findBlockPointer(const QString &name);
Wenn nicht gefunden Exception oder nullptr?
const-correctness: siehe Beispiel oben; Find-Funktionen dürften den Zustand des Objekts nicht ändern, also besser
/*! Helper function to assign the correct block to an entity */
const Block* findBlockPointer(const QString &name) const;
außer ein veränderlicher Zeiger wird wirklich benötigt. Das schaue ich mir in den betroffenen Algorithmen nochmal im Detail an. Grundsätzlich würde ich immer erst const-Funktionen bauen, und im Notfall mit (gut dokumentiertem) const-cast mir die Schreibbarkeit holen. Das vermeidet eher unbeabsichtigte Fehler.
Styleguide-Einstellungen im Qt Creator sind nicht 100% die nach CodeStyle-Dokument, was zu Änderungen in Zeilen führt (durch veränderte Einrückung), die nicht geändert wurden.
Inhaltlich prüfe ich das nachher noch (auch die Kompilierfähigkeit) - ich möchte mir zumindest die Fehlerfallbehandlungen nochmal im Detail anschauen, sodass bei nicht gefundenen Objekten (nullptr) oder in den Zeiger-basierten Algorithmen keine segfaults auftreten.
Auf den ersten Blick aber ok soweit.
Die formalen Anmerkungen oben können ja in den Pull-Request noch einfließen, d.h. den dann neuen Commit zum Pullrequest hinzufügen.
@hirseboy kannst Du bitte mal schauen, inwieweit wir eine CI Action in github definieren können, die automatisch das Kompilieren eines pullrequest-nach-merge auf Linux und Windows testet? Da gibt's schon fertige Formeln, hab jetzt aber keine Zeit, das selbst einzurichten.
Hab den Code mal kurz überflogen.
Ein paar formale Anmerkungen:
Quelltextkommentare (Variablen, Funktionen etc.) fehlen noch allerorts. Wäre nett, hier zumindest die wichtigsten Annahmen zu beschreiben. Auch innerhalb der neuen Algorithmen werden die grundlegenden Schritte nicht dokumentiert. Das macht es späteren Entwicklern schwerer, hier schnell mal Änderungen zu machen, ohne was kaputt zu machen. Bitte kommentiert die Algorithmen und verwendeten Datenstrukturen bitte wenigstens grundlegend ("was mache ich in welcher Reihenfolge?").
Bei Dokumentation im Header die Doxygen-Doku verwenden,
Wichtige wäre es, die Rückgabewerte/Fehlerbehandlung zu dokumentieren, bspw.
Wenn nicht gefunden Exception oder nullptr?
const-correctness: siehe Beispiel oben; Find-Funktionen dürften den Zustand des Objekts nicht ändern, also besser
außer ein veränderlicher Zeiger wird wirklich benötigt. Das schaue ich mir in den betroffenen Algorithmen nochmal im Detail an. Grundsätzlich würde ich immer erst const-Funktionen bauen, und im Notfall mit (gut dokumentiertem) const-cast mir die Schreibbarkeit holen. Das vermeidet eher unbeabsichtigte Fehler.
Styleguide-Einstellungen im Qt Creator sind nicht 100% die nach CodeStyle-Dokument, was zu Änderungen in Zeilen führt (durch veränderte Einrückung), die nicht geändert wurden.
Inhaltlich prüfe ich das nachher noch (auch die Kompilierfähigkeit) - ich möchte mir zumindest die Fehlerfallbehandlungen nochmal im Detail anschauen, sodass bei nicht gefundenen Objekten (nullptr) oder in den Zeiger-basierten Algorithmen keine segfaults auftreten.
Auf den ersten Blick aber ok soweit.
Die formalen Anmerkungen oben können ja in den Pull-Request noch einfließen, d.h. den dann neuen Commit zum Pullrequest hinzufügen.
@hirseboy kannst Du bitte mal schauen, inwieweit wir eine CI Action in github definieren können, die automatisch das Kompilieren eines pullrequest-nach-merge auf Linux und Windows testet? Da gibt's schon fertige Formeln, hab jetzt aber keine Zeit, das selbst einzurichten.