Anmerkungen zum Design

Sid73

Hallo!

Ich hätte gern kritische Anmerkungen zu folgendem Designentwurf (Tetris in vereinfachter Ausführung, also kein entfernen von vollständig gefüllten Zeilen und rundenbasiert). Das Ganze war ein Programmiertest im Rahmen einer Bewerbung bei einer Firma.

Klasse für Spielsteine

#include "global.h"

#pragma once

class Piece
{
private:
	shape array[4][4];

public:
	typedef void (Piece::*PPieceFunction)(void);

	Piece(void);
	~Piece(void);

	void initForm();
	void rotateLeft(void);
	void rotateRight(void);
	bool isFieldPart(int x, int y);
	bool rowIsEmpty(int x);

};

Klasse für Spielfeld

#include "piece.h"

#pragma once

const int dimBoardWidth = 9;
const int dimBoardHeight = 20;

const int boardEmptyField = 0;
const int currentStone = 1;
const int boundedStone = 2;

class Board
{	
private:
	int gameBoard[dimBoardWidth][dimBoardHeight];
	int posX, posY; // Position der unteren linken Ecke des Steines
	bool gameOver;

	bool isDropDownPossible(Piece stone);
	bool isMoveLeftPossible(Piece stone);
	bool isMoveRightPossible(Piece stone);
	bool isRotatePossible(Piece stone, Piece::PPieceFunction pFunction);

	void drawPiece (Piece stone, const int paintAction);

public:
	Board(void);
	~Board(void);

	bool getGameState(void) { return gameOver; }

	void moveLeft(Piece stone);
	void moveRight(Piece stone);

	void stoneDropIn(Piece& stone);
	void dropDown(Piece& stone);
	void rotateLeft(Piece& stone);
	void rotateRight(Piece& stone);

	void showBoard(void);

};

Daraus habe ich dann im Hauptprogramm die restlichen Funktion erstellt (drehen, runter, links, rechts, usw.). Was sagt Ihr zu dem Entwurf? Kann man daran was optimieren und wenn „ja“, was? Die Firma meinte, mein Entwurf sei nicht optimal, aber ich sehe nicht unbedingt einen besseren Weg.

Gruß,
Sid73

Warum isDropDownPossible isMoveLeftPossible und isMoveRightPossible, statt isMovePossible mit der neuen Position?
Das selbe gilt für die ganzen Move Methoden.

Blue-Tiger

in C++ gibts kein

int funktionOhneParameter(void);

sondern nur ein

int funktionOhneParameter();  // kein "void" in den Klammern

Nur als kleiner Hinweis.

Ansonsten uebergibst du viel zu viele Werte by-Value, die du wahrscheinlich als const-Referenzen uebergeben solltest. Das hat zwar alles nix mit Design zu tun, aber doch mit ordentlichem C++

EDIT: ganz allgemein scheinst du "const" nicht zu moegen

audacia

Blue-Tiger schrieb:

in C++ gibts kein

int funktionOhneParameter(void);

sondern nur ein

int funktionOhneParameter();  // kein "void" in den Klammern

Was absolut unverbindlich sowie Geschmackssache ist. Und wenn man gleichzeitig C verwendet, wird man mit leeren Klammern schon mal auf Probleme stoßen.

rüdiger

1. Nimm kein #pragma once, sondern include-Guards (oder zumindest beides)
2. Achte auf Const-Correctness zB

bool getGameState(void) const { return gameOver; }
// statt
bool getGameState(void) { return gameOver; }

3. hier void drawPiece (Piece stone, const int paintAction); macht const wieder keinen Sinn.
4. würde ich eher unsigned anstelle int benutzen, wenn du keine negativen Werte benötigst.
5. wie schon gesagt. Benutz eher const-Referenzen als Copy-by-Value für nicht-Builtin Typen

Warum heißt Piece nicht Stone?

Sid73

ohkay schrieb:

Warum isDropDownPossible isMoveLeftPossible und isMoveRightPossible, statt isMovePossible mit der neuen Position?
Das selbe gilt für die ganzen Move Methoden.

Die Funktionen unterscheiden sich tatsächlich nur in drei logischen Ausdrücken. Das könnte man natürlich auch alles in eine Funktion packen. Ich halte die Unterscheidung für sinnvoll, weil so gleich am Namen klar ist, was geprüft bzw. gemacht wird (teile und herrsche und so...).

Sid73

Blue-Tiger schrieb:

Ansonsten uebergibst du viel zu viele Werte by-Value, die du wahrscheinlich als const-Referenzen uebergeben solltest. Das hat zwar alles nix mit Design zu tun, aber doch mit ordentlichem C++

Meinst Du in etwa so:

bool isMoveLeftPossible(const Piece stone);

Die Methoden mit Adressübergabe manipulieren das übergebene Objekt (stone), weshalb ich diese nicht als const übergeben kann. Ich habe zwar gelesen, dass man das Verändern in anderen Klassen vermeiden sollte, aber ich fand den anderen Weg (abfragen von Stautsvariablen und ändern im Hauptprogramm) zu umständlich/unübersichtlich.

Sid73

rüdiger schrieb:

1. Nimm kein #pragma once, sondern include-Guards (oder zumindest beides)

Gibt's da Gründe für? Inkompatibilitäten? Ich habe #pragma once genommen, weil hier im Forum mal ein Fall diskutiert wurde, wo die include-Guards zu einen (schwierig zu findenden) Fehler geführt haben.

Warum heißt Piece nicht Stone?

In der Beschreibung stand etwas von "While the pieces are falling..." - ich dachte es erhöht den Wiedererkennungseffekt...

CStoll

Er sagte doch "const-Referenz": bool isMoveLeftPossible(const Piece& stone); .

Ansonsten scheint mir das Vorgehen etwas inkonsistent - was genau machen die Funktionen Piece::rotateLeft() und Piece::rotateRight()? Ich hätte, wenn überhaupt, alle Bewegungen im Piece implementiert und auch dort die aktuelle Position gespeichert.

Sid73

CStoll schrieb:

Er sagte doch "const-Referenz": bool isMoveLeftPossible(const Piece& stone); .

ähem Die Idee hatte ich auch, da ja an dem Objekt nichts verändert wird, aber wenn ich das so mache, bekomme ich folgenden Fehler:

'Piece::isFieldPart': this-Zeiger kann nicht von 'const Piece' in 'Piece &' konvertiert werden

isFieldPart(x,y) prüft ob das Feld in der 4x4-Matrix (des Steins) belegt ist und liefert bool zurück. Das hat den Sinn nur genau die Felder zur prüfen, die bei entsprechender Bewegung belegt sein würden. Warum bekomme ich den Fehler, wo sich doch an der Adresse nichts ändert?

Ansonsten scheint mir das Vorgehen etwas inkonsistent - was genau machen die Funktionen Piece::rotateLeft() und Piece::rotateRight()? Ich hätte, wenn überhaupt, alle Bewegungen im Piece implementiert und auch dort die aktuelle Position gespeichert.

Die Funktionen drehen den Stein um die z-Achse (d.h. Zeilen und Spalten werden vertauscht) in die jeweilige Richtung. Der Aufruf sieht dann etwa so aus

void Board::rotateLeft(Piece& stone)
{	
	if (isRotatePossible(stone, &Piece::rotateLeft))
	{
		drawPiece(stone, boardEmptyField);
		stone.rotateLeft();
		drawPiece(stone, currentStone);
	}
}

Die Drehung selbst ist ja auch in Piece implementiert, aber ich muß ja über das Board abfragen, ob das nicht mit schon vorhandenen Steinen kollidiert bzw. ob überhaupt Platz zum drehen da ist!?

Th

Du hast die Funktion isFieldPart ja auch nicht als const deklariert:

bool isFieldPart(int x, int y) const;

Dies zusammen mit der Übergabe mittels "const T&" nennt man const-correctness, d.h. du kannst somit nur auf konstante Methoden bzw. Member zugreifen.

CStoll

Netter Ansatz, aber wie gesagt - ich hätte die Steinbewegungen einheitlicher umgesetzt. Sprich: Der Stein merkt sich auch sein aktuelle Position und bietet auch Methoden zum Verschieben und Fallen, so daß du nur noch eine Funktion isMovePossible(Piece,Piece::PPieceFunction) und MovePiece(Piece&,Piece::PPieceFunction) benötigst.

tntnet

Sid73 schrieb:

rüdiger schrieb:

1. Nimm kein #pragma once, sondern include-Guards (oder zumindest beides)

Gibt's da Gründe für? Inkompatibilitäten? Ich habe #pragma once genommen, weil hier im Forum mal ein Fall diskutiert wurde, wo die include-Guards zu einen (schwierig zu findenden) Fehler geführt haben.

Dieses "#pragma once" ist die Erfindung eines Compilerherstellers, die unnötig ist, da das gleiche mit include-Guards standardkonform gelöst werden kann. Grundsätzlich sollte man ohne guten Grund nicht von ANSI-C++ abweichen.

camper

tntnet schrieb:

Sid73 schrieb:

rüdiger schrieb:

1. Nimm kein #pragma once, sondern include-Guards (oder zumindest beides)

Gibt's da Gründe für? Inkompatibilitäten? Ich habe #pragma once genommen, weil hier im Forum mal ein Fall diskutiert wurde, wo die include-Guards zu einen (schwierig zu findenden) Fehler geführt haben.

Dieses "#pragma once" ist die Erfindung eines Compilerherstellers, die unnötig ist, da das gleiche mit include-Guards standardkonform gelöst werden kann. Grundsätzlich sollte man ohne guten Grund nicht von ANSI-C++ abweichen.

Das ist allerdings keine Abweichung vom Standard. pragma Direktiven sind zwar implementationsabhängig, beeinträchtigen aber nicht die Konformität. Es ist daher auch nicht notwendig, das ganze mit einem #if zu umschließen, damit bei anderen Compilern nichts passiert - höchtens um sicherzugehen, dass diese die Direktive nicht falsch verstehen. Nicht standardkonform wäre allerdings wohl der Verzicht auf Includeguards, wenn diese ohne #pragma once notwendig wären.

Sid73

CStoll schrieb:

Netter Ansatz, aber wie gesagt - ich hätte die Steinbewegungen einheitlicher umgesetzt. Sprich: Der Stein merkt sich auch sein aktuelle Position und bietet auch Methoden zum Verschieben und Fallen, so daß du nur noch eine Funktion isMovePossible(Piece,Piece::PPieceFunction) und MovePiece(Piece&,Piece::PPieceFunction) benötigst.

Also nur nochmal für mich zum Verständnis: Du würdest lediglich eine Klasse für die Spielsteine nehmen und die Board-Klasse einfach als Matrix mit ein paar Funktionen im Hauptprogramm abbilden!?

Ansonsten danke an alle für Eure Anmerkungen!

CStoll

Vermutlich ja. Oder ich würde alle Aktionen in die Spielfeld-Klasse packen und die Steine zu Datenobjekten "degradieren". Aber auf jeden Fall würde die Kompetenzen klar aufteilen.