So, fangen wir mal wegen der Kommentare, hatte ich ja gesagt ich mich darum bemühe diese sachlich und kritiisch zu hinterfragen:
Dabei habe ich versucht Dinge nicht zu hinterfragen, die für andere "selbstverständlich sein könnten", wobei so fast....
Turn.cpp
Zeile 6:
Wofür wird "
Turn::instances" genutzt?
Die Erklärung dass die Variable als "static" deklariert worden ist zielführend, sagt aber nichts über den Nutzen. Auch spricht die Variable nicht direkt, bzw. verstehe ich den Zusammenhang nicht was diese später tun soll oder welche Wirkung sie hat.
Zeile 10-14:
Die Funktion "Turn:: Turn" hat als Beschreibung "Brief: Custom Constructor", sollte da nicht dann eher stehen "Advances to the next turn" oder so etwas? Knüpft aber auch direkt an das vorhergehende an, da dort die mysteriöse "instance" Variable genutzt wird.
Zeile 33 bzw. 30 - "Turn::setTarget":
hier werde ich nicht 100% schlau aus der Beschreibung, ist das "Target" in dem Fall das Feld auf den gesetzt wird?
Zeile 57: "Turn::getTarget":
gleiches Problem in der Beschreibung wie mit SetTarget, was ist "Target": "retrieves the players played field (target) in turn"?
Group.cpp
Zeile 36, "Group::getColor" : @brief Return the color of the group << Okay, aber hier wäre noch interessant zu erwähnen, das es sich um die "Spieler"farbe handelt, nicht nur um eine Farbe... diese Auslegung ist so nicht wirklich eindeutig.
Und vielleicht ebenfalls für "Group::setColor", Zeile 97 - das Wort "Spieler" mit einbringen.
Board.cpp
https://github.com/roin93/GoStatistics/blob/master/Board.cpp#L12: "InitPoints"
hier könnte man noch eine Beschreibung geben, das diese Funktion die Punkte der Spiele initialisiert, es ist zwar durch den Funktionsnamen recht verständlich, aber ein "Initializes the player points according to game rules" wäre nicht verkehrt.
Also bei "ApplyTurn" (erweitert und vereinfacht) - hast du ja schon sehr viel kommentiert. Das einzige was mir auffällt, kann man die Reihenfolge der Funktionen anders arrangieren?
Zum Beispiel, du rufst in "ApplyTurn" (erweitert) die Funktion "applyTurnToGroups" auf (Zeile 48). Diese wurde aber noch nicht beschrieben vorher.
Nachdem ich weiß was die Groups machen, kann ich mir auch denken was die Funktion macht, aber es ist trotz Kommentar in Zeile 47: "// Add the new stone to the Groups"
Vielleicht könnte man es so schreiben: Ein Stein wird einer existierenden Gruppe hinzugefügt ("oder eine neue Gruppe erstellt?")
Macht glaube ich Sinn was ich meine, da ich die Funktionsbeschreibung noch nicht gelesen habe, sehe ich die Funktion im Einsatz, aber hab keine Begrifflichkeiten dazu.
Deßhalb, wenn man die Reihenfolge etwas anpassen kann...
Zeile 64 und 65 in der Funktion:
"""
m_numberOfRemovedStonesLastTurn = 0;
m_removedStonesLastTurn.clear();
"""
Da bin ich raus, warum brauchen wir die Angabe was den "vorherigen" Zug angeht bzw. warum wird das gespeichert? Vielleicht wird das später noch klar, aber aktuell verstehe ich das große Bild dahinter nicht. Sind die Variablen für die Statistik? Etwas unklar.
Und nach Zeile 67:
if(!catchedGroups.empty())
schreibst du:
// Save the removed Stones to check if an Atari is given for the next Turn
Also, du speicherst die Steine die entfernt werden, um zu prüfen ob im "nächsten" (?) Zug ein Atari kommt?
Zeile 138 ist ein meiner Meinung nach etwas kryptisch, also fast ein schöner "OneLiner"
if(not (*neighbourGroups.begin())->addMember(t.getTarget()))
Was evaluiert diese Funktion? - Sie gibt -1 zurück.
Ich weiß es steht in der Beschreibung der Funktion was das -1 bedeutet, aber ein Kommentar wäre für mich nicht verkehrt an genau so einer Stelle Code.
Und auch was der Rückgabewert dann innerhalb des "if" Blocks bedeutet. Der "else" Block des gleichen "if" fehlt auch eine kurze Notiz.
Zeile 178: "Board::changeColor" und Zeile 187 "Board::getColor" - vielleicht auch hier wieder "Spieler"farbe, anstelle nur das Wort "Farbe" zu benutzen?
Zeile 211, "Board::getCatchedGroups": Auch das "Brief:" Liefert die Gruppen die von einer "befreundeten Spieler"farbe umschlossen sind.
Das befreundet habe ich aus dem Param für Color, aber in der generell Beschreibung könnte man diesen "Twist" (das die Erklärung danach kommt durch den "color" Parameter) entfernen.
Zeile 239:
hasFreedoms = true; // Die Gruppe ist nicht gefangen?
Zeile 296, "Board::getMaxX"
Würdest du hier nicht ergänzen wollen, das die Funktion die "maximale Spielfeldbreite" (in der X-Achse) liefert?
Könnte man dann aber bei"getMaxY", "getMinX", getMinY" auch vielleicht entsprechend ergänzen - wie auch bei den "Setter" Funktionen.
Zeile 715: "Board::addPoints" - ich glaube diese und die darauf folgenden Funktionen sind später dazugekommen, oder? Hier ist noch nichts kommentiert. Wollte das nur anmerken.
So, ich glaube das reicht auch erst einmal, falls ich einige Anmerkungen zu oft wiederholt habe oder gebetsmühlenartig klinge, tuts mir leid.