- Registriert
- 1 Aug. 2013
- Beiträge
- 235
@theSplit
Ich sehe auf GitHub kein 1-dim. Array, sondern min/max in 2-dim (x und y). Das passt problemlos mit Coordinate zusammen.
Die Nachbar-Funktionalität ist aus meiner Sicht beim Board besser aufgehoben, weil nur gültige – im Rahmen des Spielbrett liegende – Nachbarn interessant sind. Aber von den Spielbrettdimensionen weiß Coordinate nichts. Wenn ihr’s so macht, wird Coordinate dann auch zu einem richtig hübschen, kleinen, schlanken, dummen, geradezu trivialen Datencontainer.
… Hm, eigentlich wollte ich auf GitHub nur mal kurz reinschauen, und dann stand plötzlich die dumme Variante von Coordinate im Texteditor, so wie ich sie bauen würde. Dann kann ich jetzt auch ein paar Worte dazu schreiben.
Der Übersicht halber alles direkt inline implementiert. Würde ich bei so einer Miniklasse vermutlich im Produktivcode genauso machen.
[src=cpp]
// file: Coordinate.hpp
class Coordinate {
public:
Coordinate() = default;
Coordinate(int x, int y): m_x(x), m_y
{}
int x() const { return m_x; }
int y() const { return m_y; }
private:
int m_x = 0;
int m_y = 0;
}
inline bool operator==(const Coordinate& lhs, const Coordinate& rhs)
{
return lhs.x() == rhs.x() && lhs.y() == rhs.y();
}
inline bool operator!=(const Coordinate& lhs, const Coordinate& rhs)
{
return not (lhs == rhs);
}
[/src]
Was mir beim Drüberlesen auf GitHub sonst noch aufgefallen ist:
So. Puh. Lang geworden. Sorry, ist halt C++. Wenn ich da einmal in Fahrt bin … Wenn das zu viel Review vom Außenseiter war, dann bremst mich. Oder wenn ihr mehr davon wollt, sagts.
Ich sehe auf GitHub kein 1-dim. Array, sondern min/max in 2-dim (x und y). Das passt problemlos mit Coordinate zusammen.
Die Nachbar-Funktionalität ist aus meiner Sicht beim Board besser aufgehoben, weil nur gültige – im Rahmen des Spielbrett liegende – Nachbarn interessant sind. Aber von den Spielbrettdimensionen weiß Coordinate nichts. Wenn ihr’s so macht, wird Coordinate dann auch zu einem richtig hübschen, kleinen, schlanken, dummen, geradezu trivialen Datencontainer.
… Hm, eigentlich wollte ich auf GitHub nur mal kurz reinschauen, und dann stand plötzlich die dumme Variante von Coordinate im Texteditor, so wie ich sie bauen würde. Dann kann ich jetzt auch ein paar Worte dazu schreiben.
Der Übersicht halber alles direkt inline implementiert. Würde ich bei so einer Miniklasse vermutlich im Produktivcode genauso machen.
[src=cpp]
// file: Coordinate.hpp
class Coordinate {
public:
Coordinate() = default;
Coordinate(int x, int y): m_x(x), m_y
{}int x() const { return m_x; }
int y() const { return m_y; }
private:
int m_x = 0;
int m_y = 0;
}
inline bool operator==(const Coordinate& lhs, const Coordinate& rhs)
{
return lhs.x() == rhs.x() && lhs.y() == rhs.y();
}
inline bool operator!=(const Coordinate& lhs, const Coordinate& rhs)
{
return not (lhs == rhs);
}
[/src]
- Ich fand’s in der GitHub-Variante einen sehr feinen Ansatz, Coordinate als value type, also immutable, anzulegen. Eine Koordinate ist ja nicht besonders viel mehr als ein Integer. Dass sie sich verhält wie ein eigebauter Fundamentaltyp, passt perfekt ins Bild. Unschönheit auf GitHub in der Hinsicht: die Getter und Neighbour-Funktionen sind nicht const, was sie aber sowieso und beim value type erst recht sein könnten und sollten. Non-const-Methoden machen für einen immutable Typen keinen Sinn.
- Warum stehen da oben genau diese Konstruktoren? Der Custom-Ctor mit den beiden ints verhindert, dass der Compiler automatisch den parameterlosen Default-Ctor generiert. Das =default sagt dem Compiler ausdrücklich, dass er den doch generieren soll. Copy-/Move-Ctors und Copy-/Move-Assignment sind compilergeneriert. D.h. den manuellen Copy-Ctor wie auf GitHub brauchts nicht. Schaut mal weiter runter für die kompletten Regeln, wann welche Ctors/Zuweisungsoperatoren automatisch erzeugt werden.
- Standardoperatoren kommen selten allein. Es bietet sich immer an, alle einer Gruppe zu implementieren, also z.B. operator==() und operator!=(). Sonst kommt man in so seltsame Situationen, dass c1 == c2 compiliert, c1 != c2 aber nicht.
- Die meisten Operatoren implementiert man besser als freie Funktionen. Als Member wie auf GitHub ist jetzt definitiv, dass der erste Parameter eines Vergleichs ein Coordinate, und *nur* ein Coordinate, sein muss. Die freie Variante wie oben erlaubt für beide Parameter auch andere Typen, die implizit zu einem Coordinate konvertierbar sind.
Code:
horizontal: if you write ...
vertical: the compiler generates ...
none dtor copy ctor copy op= move ctor move op=
dtor yes yes yes yes yes
copy ctor yes yes yes no no
copy op= yes yes yes no no
move ctor yes no no no no
move op= yes no no no noWas mir beim Drüberlesen auf GitHub sonst noch aufgefallen ist:
- Da wird stdio.h includiert, obwohl weit und breit kein I/O zu sehen ist. Davon abgesehen ist’s ohne Not ein C-Header (in C++ heißt das Ding cstdio). Außerdem würde ich in einem C++-Programm tendenziell auch C++-I/O erwarten, also iostream.
- Member mit __ anfangen zu lassen, ist potenziell gefährlich, weil solche Namen für die Implementierung (von Compiler und Standardlib) reserviert sind. Kann also, wenns blöd läuft, zu Namensclashes führen. Daumenregel: in Namen keine führenden Unterstriche und keine Mehrfachunterstriche.
- Das *using std::vector;* im Header hat virale Wirkung auf alle Translation Units, die diesen Header einbinden. Sollte man vermeiden. Früher oder später clasht sowas mal böse.
So. Puh. Lang geworden. Sorry, ist halt C++. Wenn ich da einmal in Fahrt bin … Wenn das zu viel Review vom Außenseiter war, dann bremst mich. Oder wenn ihr mehr davon wollt, sagts.
.