Optimierung und Tipps anhand von Snake

[Zero07]

Hi Zusammen,
ich hab heute mal Snake programmiert, allerdings bin ich mir oft nicht sicher ob ich meine Aufgabe "schön" löse. Ich wollte mal hören was ihr zur Lösung sagt und ob ihr ein paar allgemeine Tipps zur Optimierung für mich hättet?!? Auch wenn es sich eigentlich um ein simples Programm handelt, kommt es mir so vor, als hätte ich viel zu viel Code...

Snake.h

#ifndef _SNAKE_H_
#define _SNAKE_H_

#include <queue>
#include <time.h>
#include <stdlib.h>
#include <conio.h>
#include <iostream>
#include <Windows.h>
using namespace std;

const short breadth = 40;	// Breite inkl. Spielfeldgrenze
const short height = 20;	// Höhe inkl. Spielfeldgrenze
const short startPos_x = 5;
const short startPos_y = 5;
const short speed = 100;
const char iconBody = 'O';
const char iconFood = '#';
const char blank = char(0);

class Snake {
public:
	enum arrowKeys {LEFT = 75, UP = 72, RIGHT = 77, DOWN = 80};

private:
	// private Attribute
	short points;		      // speichert die aktuelle Punktzahl
	short field[height][breadth]; // speichert Information des Spielfeldes
				      // Spielfeldgrenze := field[y][x] == -1 
				      // Schlange	 := field[y][x] ==  1
				      // Essen		 := field[y][x] ==  2
				      // freies Feld     := field[y][x] ==  0
	HANDLE console;

	COORD currentPosition;
	COORD head;
	COORD tail;
	COORD food;

	arrowKeys currentDirection;	// speichert die aktuelle Richtung der Schlange
	queue<COORD> snakeCoordinates;	// speichert die aktuellen Koordinaten der Schlange

	// private Methoden
	void setConsole();

	const short getFieldSize_x() const {return breadth-2;}	// ermittelt die Breite ohne Spielfeldgrenze
	const short getFieldSize_y() const {return height-2;}	// ermittelt die Höhe ohne Spielfeldgrenze

	void initializeHead(short x, short y);
	void initializeField();

	arrowKeys getCurrentDirection() const {return currentDirection;}

	void gotoxy(short x, short y);
	void printBorder() const;

public:
	// öffentliche Methoden
	Snake();
	COORD getHead() const {return head;}
	void moveHead();

	COORD getTail() const {return tail;}
	void moveTail();

	void setRandomFood();

	void increasePoints();

	void updateFieldInformation(COORD pos, short value) {field[pos.Y][pos.X] = value;}

	void sleep() {Sleep(speed);}

	void print(COORD pos, const char icon);
	void updateSnakeCoordinates(COORD s) {snakeCoordinates.push(s);}

	void updateCurrentDirection();
	bool collision() const;
	bool eatFood() const;
	void gameOver();
};

#endif _SNAKE_H_

Snake.cpp

#include "Snake.h"	

Snake::Snake() : points(0), currentDirection(RIGHT) {
	srand((unsigned int)(time(NULL)));

	SetConsoleTitle("Snake");
	setConsole();
	printBorder();

	initializeHead(startPos_x, startPos_y);
	tail = head;
	setRandomFood();

	initializeField();
	gotoxy(27, height);
	cout << "Punkte: " << points;
}

void Snake::setConsole() {
	console = GetStdHandle(STD_OUTPUT_HANDLE);
}

void Snake::initializeHead(short x, short y) {
	head.X = x;
	head.Y = y;
}

void Snake::moveHead() {
	switch(getCurrentDirection()) {
	case LEFT:
		head.X--;
		break;
	case UP:
		head.Y--;
		break;
	case RIGHT:
		head.X++;
		break;
	case DOWN:
		head.Y++;
		break;
	}
}

void Snake::moveTail() {
	tail = snakeCoordinates.front();
	snakeCoordinates.pop();
}

void Snake::setRandomFood() {
	bool freePos = false;

	while(!freePos) {
		food.X = rand() % (getFieldSize_x())+1;
		food.Y = rand() % (getFieldSize_y())+1;

		if(field[food.Y][food.X] != 1) {
			freePos = true;
		}

	}
	gotoxy(food.X, food.Y);
	cout << iconFood;

	field[food.Y][food.X] = 2;
}

void Snake::increasePoints() {
	points += 100;

	gotoxy(35, height);
	cout << points;
}

void Snake::initializeField() {
	for(int i = 0;i < height;i++) {
		for(int j = 0;j < breadth;j++) {
			if(i == 0 || i == height-1) {
				field[i][j] = -1;
			}
			else if(j == 0 || j == breadth-1) {
				field[i][j] = -1;
			}
			else {
				field[i][j] = 0;
			}
		}
	}
	field[food.Y][food.X] = 2;
}

void Snake::updateCurrentDirection() {
	if(_getch() == 224) {
		if(currentDirection == LEFT || currentDirection == RIGHT) {
			switch(_getch()) {
			case DOWN:
				currentDirection = DOWN;
				break;
			case UP:
				currentDirection = UP;
				break;
			}
		}
		else {
			switch(_getch()) {
			case LEFT:
				currentDirection = LEFT;
				break;
			case RIGHT:
				currentDirection = RIGHT;
				break;
			}
		}
	}	
}

void Snake::print(COORD pos, const char icon) {
	gotoxy(pos.X, pos.Y);
	cout << icon;
}

void Snake::gotoxy(short x, short y) {
	currentPosition.X = x;
	currentPosition.Y = y;

	SetConsoleCursorPosition(console, currentPosition);
}

bool Snake::collision() const {
	if(field[head.Y][head.X] == -1 || field[head.Y][head.X] == 1) {
		return true;
	}
	else {
		return false;
	}
}

bool Snake::eatFood() const {
	if(field[head.Y][head.X] == 2) {
		return true;
	}
	else {
		return false;
	}
}

void Snake::printBorder() const {
	for(int i = 1;i <= height;i++) {
		for(int j = 1;j <= breadth;j++) {
			if(i == 1) {
				if(j == 1) {
					cout << char(201);
				}
				else if(j == breadth) {
					cout << char(187);
				}
				else {
					cout << char(205);
				}
			}
			else if(i == height) {
				if(j == 1) {
					cout << char(200);
				}
				else if(j == breadth) {
					cout << char(188);
				}
				else {
					cout << char(205);
				}
			}
			else if(j == 1 || j == breadth) {
				cout << char(186);
			}
			else {
				cout << blank;
			}
		}
		cout << endl;
	}
}

void Snake::gameOver() {
	gotoxy(0, height);
	cout << "Game Over!";

	gotoxy(0, height+1);
	system("pause");
}

main.cpp

#include "Snake.h"

int main() {
	Snake snake;

	while(true) {
		while(!_kbhit()) {
			snake.moveHead();

			if(snake.collision()) {
				snake.gameOver();
				exit(true);
			}
			else if(snake.eatFood()) {
				snake.updateSnakeCoordinates(snake.getHead());
				snake.setRandomFood();
				snake.increasePoints();
			}

			snake.updateFieldInformation(snake.getHead(), 1);
			snake.print(snake.getHead(), iconBody);

			snake.sleep();

			snake.updateSnakeCoordinates(snake.getHead());
			snake.moveTail();
			snake.updateFieldInformation(snake.getTail(), 0);
			snake.print(snake.getTail(), blank);
		}
		snake.updateCurrentDirection();
	}

	system("pause");
	return 0;
}

[rapso]

#ifndef _SNAKE_H_
#define _SNAKE_H_

#include <queue>
#include <time.h>
#include <stdlib.h>
#include <conio.h>
#include <iostream>
#include <Windows.h>
using namespace std;

const short breadth = 40;	// Breite inkl. Spielfeldgrenze
const short height = 20;	// Höhe inkl. Spielfeldgrenze
const short startPos_x = 5;
const short startPos_y = 5;
const short speed = 100;
const char iconBody = 'O';
const char iconFood = '#';
const char blank = char(0);

da das keine globalen consts sind, sondern teil vom spiel, waere es nicht verkehrt diese in die klasse einzubauen

class Snake {
public:
	enum arrowKeys {LEFT = 75, UP = 72, RIGHT = 77, DOWN = 80};

private:
	// private Attribute
	short points;		      // speichert die aktuelle Punktzahl
	short field[height][breadth]; // speichert Information des Spielfeldes
				      // Spielfeldgrenze := field[y][x] == -1 
				      // Schlange	 := field[y][x] ==  1
				      // Essen		 := field[y][x] ==  2
				      // freies Feld     := field[y][x] ==  0

das was da als comment steht, gehoert in ein enum

HANDLE console;

	COORD currentPosition;
	COORD head;
	COORD tail;
	COORD food;

	arrowKeys currentDirection;	// speichert die aktuelle Richtung der Schlange
	queue<COORD> snakeCoordinates;	// speichert die aktuellen Koordinaten der Schlange

das sind ein bissl capt obvious comments

// private Methoden
	void setConsole();

	const short getFieldSize_x() const {return breadth-2;}	// ermittelt die Breite ohne Spielfeldgrenze
	const short getFieldSize_y() const {return height-2;}	// ermittelt die Höhe ohne Spielfeldgrenze

ein wenig inkonsistente benennung, wie waere es mit getFieldSizeX?

void initializeHead(short x, short y);
	void initializeField();

	arrowKeys getCurrentDirection() const {return currentDirection;}

	void gotoxy(short x, short y);
	void printBorder() const;

public:
	// öffentliche Methoden
	Snake();
	COORD getHead() const {return head;}
	void moveHead();

	COORD getTail() const {return tail;}
	void moveTail();

	void setRandomFood();

	void increasePoints();

	void updateFieldInformation(COORD pos, short value) {field[pos.Y][pos.X] = value;}

	void sleep() {Sleep(speed);}

eventuell den sleep anhand der schon vergangenen zeit zum letzten sleep bestimmen? kann ja sein, dass ein system langsam ist und dann wuerde das spiel langsammer laufen, natuerlich fuer ein text snake nicht die rede, nur generell.

void print(COORD pos, const char icon);
	void updateSnakeCoordinates(COORD s) {snakeCoordinates.push(s);}

	void updateCurrentDirection();
	bool collision() const;
	bool eatFood() const;
	void gameOver();
};

#endif _SNAKE_H_

#include "Snake.h"	

Snake::Snake() : points(0), currentDirection(RIGHT) {
	srand((unsigned int)(time(NULL)));

	SetConsoleTitle("Snake");
	setConsole();

das nachfolgende wuerde ich in eine extra funktion stecken, weil es doch eigentlich so sein sollte, dass man mehrfach spielen wuerde, oder?

printBorder();

	initializeHead(startPos_x, startPos_y);
	tail = head;

	setRandomFood();

	initializeField();
	gotoxy(27, height);
	cout << "Punkte: " << points;
}

27?magic number, vielleicht mit in die deklaration der consts oben?

void Snake::setConsole() {
	console = GetStdHandle(STD_OUTPUT_HANDLE);
}

wuerde es nicht reichen das im ctor zu machen? ist zwar nicht verkehrt, aber ich sehe nicht weeshalb das seperat muss, wird es jemals von aussen aufgerufen oder nach dem ctor?

void Snake::initializeHead(short x, short y) {
	head.X = x;
	head.Y = y;
}

void Snake::moveHead() {
	switch(getCurrentDirection()) {
	case LEFT:
		head.X--;
		break;
	case UP:
		head.Y--;
		break;
	case RIGHT:
		head.X++;
		break;
	case DOWN:
		head.Y++;
		break;
	}
}

falls dir code size zuviel ist, dann koentest du "direction" auch einfach als 2d vector nehmen, statt den case block

void Snake::moveTail() {
	tail = snakeCoordinates.front();
	snakeCoordinates.pop();
}

void Snake::setRandomFood() {
	bool freePos = false;

	while(!freePos) {
		food.X = rand() % (getFieldSize_x())+1;
		food.Y = rand() % (getFieldSize_y())+1;

		if(field[food.Y][food.X] != 1) {
			freePos = true;
		}

	}
	gotoxy(food.X, food.Y);
	cout << iconFood;

	field[food.Y][food.X] = 2;
}

void Snake::increasePoints() {
	points += 100;

	gotoxy(35, height);
	cout << points;
}

+100? 35? -> consts

void Snake::initializeField() {
	for(int i = 0;i < height;i++) {
		for(int j = 0;j < breadth;j++) {
			if(i == 0 || i == height-1) {
				field[i][j] = -1;
			}
			else if(j == 0 || j == breadth-1) {
				field[i][j] = -1;
			}
			else {
				field[i][j] = 0;
			}
		}
	}
	field[food.Y][food.X] = 2;
}

statt if if else, vielleicht nur den ?: operator verwenden

void Snake::updateCurrentDirection() {
	if(_getch() == 224) {

224 magic?

if(currentDirection == LEFT || currentDirection == RIGHT) {
			switch(_getch()) {
			case DOWN:
				currentDirection = DOWN;
				break;
			case UP:
				currentDirection = UP;
				break;
			}
		}
		else {
			switch(_getch()) {
			case LEFT:
				currentDirection = LEFT;
				break;
			case RIGHT:
				currentDirection = RIGHT;
				break;
			}
		}
	}	
}

void Snake::print(COORD pos, const char icon) {
	gotoxy(pos.X, pos.Y);
	cout << icon;
}

void Snake::gotoxy(short x, short y) {
	currentPosition.X = x;
	currentPosition.Y = y;

	SetConsoleCursorPosition(console, currentPosition);
}

bool Snake::collision() const {
	if(field[head.Y][head.X] == -1 || field[head.Y][head.X] == 1) {
		return true;
	}
	else {
		return false;
	}
}

bool Snake::eatFood() const {
	if(field[head.Y][head.X] == 2) {
		return true;
	}
	else {
		return false;
	}
}

void Snake::printBorder() const {
	for(int i = 1;i <= height;i++) {
		for(int j = 1;j <= breadth;j++) {
			if(i == 1) {
				if(j == 1) {
					cout << char(201);
				}
				else if(j == breadth) {
					cout << char(187);
				}
				else {
					cout << char(205);
				}
			}
			else if(i == height) {
				if(j == 1) {
					cout << char(200);
				}
				else if(j == breadth) {
					cout << char(188);
				}
				else {
					cout << char(205);
				}
			}
			else if(j == 1 || j == breadth) {
				cout << char(186);
			}
			else {
				cout << blank;
			}
		}
		cout << endl;
	}
}

border ist doch im field drinne, oder? theoretisch koenntest du doch einen kleinen loop machen wo snake, food, border etc, zugleich abgearbeitet wird.

void Snake::gameOver() {
	gotoxy(0, height);
	cout << "Game Over!";

	gotoxy(0, height+1);
	system("pause");
}

main.cpp

#include "Snake.h"

int main() {
	Snake snake;

	while(true) {
		while(!_kbhit()) {
			snake.moveHead();

			if(snake.collision()) {
				snake.gameOver();
				exit(true);
			}
			else if(snake.eatFood()) {
				snake.updateSnakeCoordinates(snake.getHead());
				snake.setRandomFood();
				snake.increasePoints();
			}

			snake.updateFieldInformation(snake.getHead(), 1);
			snake.print(snake.getHead(), iconBody);

			snake.sleep();

			snake.updateSnakeCoordinates(snake.getHead());
			snake.moveTail();
			snake.updateFieldInformation(snake.getTail(), 0);
			snake.print(snake.getTail(), blank);
		}
		snake.updateCurrentDirection();
	}

	system("pause");
	return 0;
}

[/quote]

an sich recht gut. man sieht dass du es ordentlich machen willst

deine namensgebung ist ein wenig inkonsistent, manchmal hast du "getFieldSize" dann "collision", set/get funktionen sind eher c style, weil man nicht zweimal denselben namen nutzen konnte, in c++ wuerde ich setter/getter funktionen so wie deine "collision" funktion, ohne "get" davor machen. "FieldSizeX" waere meiner meinung nach ziemlich eingaengig.
zugleich sollte man versuchen sogenante "doit" funktionen zu meiden, wo zwar jeder weiss, dass was gemacht wird, aber niemand so wirklich was. "setRandomFood" waere also vielleicht als "placeRandomFood" besser, eventuell sogar "placeBait"

einige deiner kommentare sind nicht noetig, bzw nur da weil der variablenname nicht aussagefaehig genug ist. statt z.b. breadth und dann zu sagen // ist die feld breite, nenn es lieber gleich "FieldWidth", statt points und dann //speichert punktzahl, gleich "score". schau dir deinen code an, der ist zu 90% gut und verstaendlich ohne kommentare, die kommentare wirken als notloesung. wenn du also schon trvial comments schreiben willst, ueberleg dir, ob du es an den stellen nicht besser machen koenntest, denn nicht jeder liest deinen code in der reihenfolge in dem du ihn schreibst, und weiter unten steht dann z.b. "points" und dann fehlt da ein comment.

ist vielleicht ein wenig persoenliche meinung, aber du kannst es ja ignorieren :P, ich finde bei deinen if/else konstrukten wo oft nur eine zeile ist, musst du keine klammern schreiben.
was keine meinung ist: sowas wie

if(true)
return true;
else
return false

sollte man mit der zeit ausmerzen, das ist einfach nur

bool Snake::eatFood() const {
    return field[head.Y][head.X] == 2;
}

alles im guten und zur verbesserung kommentiert, nicht um dich fertig zu machen oder so. wie ich sagte, es sieht schon gut aus fuer jemanden der gerade erst snake programmiert hat. und wenn ich meinen code posten wuerde, wuerden sicherlich auch viele noch verbesserungswuerdige stellen finden, man wird nie perfekt sein.

[Zero07]

Also schonmal Danke für die schnelle Antwort und kosntruktive Kritik Ich bin ja froh das ich andere Meinungen höre, sonst hätte ich es nicht veröffentlicht
Ich geb dir recht, dass einige der Kommentare nicht sehr sinnvoll sind und um ehrlich zu sein hab ich mir bei einigen Methodennamen zuviele Gedanken gemacht.
Um das Spiel zu vervollständigen fehlt natürlich sowas wie ein Menü, habe ich jetzt nicht gemacht, da es mir nur um das Grundprinzip ging.

gotoxy(27, height);
cout << "Punkte: " << points;

void Snake::increasePoints() {
    points += 100;

    gotoxy(35, height);
    cout << points;
}

Die beiden Zahlen sind jetzt auf die aktuellen Fenstergrößen angepasst und nicht dynamisch-->verbesserungswürdig
100 einfach nur, da sich die Punktzahl bei jedem "Essen" um 100 erhöht.

224 ist doch die Zahl die beim Drücken einer Sondertaste(in dem Fall die Pfeiltasten) zurückgegeben wird oder nicht?!

Wie meinst du das mit dem 2d vector für die "direction"?

[rapso]

Zero07 schrieb:

gotoxy(27, height);
cout << "Punkte: " << points;

void Snake::increasePoints() {
    points += 100;

    gotoxy(35, height);
    cout << points;
}

Die beiden Zahlen sind jetzt auf die aktuellen Fenstergrößen angepasst und nicht dynamisch-->verbesserungswürdig
100 einfach nur, da sich die Punktzahl bei jedem "Essen" um 100 erhöht.
224 ist doch die Zahl die beim Drücken einer Sondertaste(in dem Fall die Pfeiltasten) zurückgegeben wird oder nicht?!

du weisst es, ich weiss es in diesem fall, aber generell hat es schon einen grund keine magic numbers zu haben, denn man kann sich streiten ab wann man nicht mehr weiss was eine magic zahl soll und da code meistens waechst, z.b. koenntest du ja noch einbauen dass neben den koeder noch ab und zu besonders punktreiche 'fliegen' auftauchen, fuer kurze zeit; waere es nicht intuitiv durch den code zu gehen und nach stellen zu suchen die eventuel eine zahl verwenden die man aendern will.

Wie meinst du das mit dem 2d vector für die "direction"?

void Snake::moveHead() {
    head+=currentDirection;
/*
    switch(getCurrentDirection()) {
    case LEFT:
        head.X--;
        break;
    case UP:
        head.Y--;
        break;
    case RIGHT:
        head.X++;
        break;
    case DOWN:
        head.Y++;
        break;
    }
*/
}

entsprechend

void Snake::updateCurrentDirection() {
    if(_getch() == 224) {
        if(currentDirection == LEFT || currentDirection == RIGHT) {
            switch(_getch()) {
            case DOWN:
                currentDirection = COORD(0,-1);
                break;
            case UP:
                currentDirection = COORD(0,1);
                break;
            }
        }
        else {
            switch(_getch()) {
            case LEFT:
                currentDirection = COORD(-1,0);
                break;
            case RIGHT:
                currentDirection = COORD(1,0);
                break;
            }
        }
    }    
}

wieso ist eigentlich COORD gross geschrieben? ist das ein macro oder sowas? deine "Snake" klasse folgt anderen regeln.

[Dobi]

Was du mit der 224 machst, ist schon richtig, aber wär halt nett wenn du es dranschreibst, oder gar irgendwo Sondertastencode = 224 oder so schreibst und dann auf (_getch() == Sondertastencode) prüfst.

Direction könnte eine Mini-Klasse mit x und y als Member sein und mit schöner Operatorüberladung (ruhig auch als freie Funktionen). Dazu noch eine Enum mit vier Elementen und du könntest dann bei dir sowas wie position += Direction::Up; sagen.

Edit: Mist, zu lahm.

[rapso]

Dobi schrieb:

...sowas wie position += Direction::Up; sagen

stimmt, sowas ist besser also meine magic numbers, also sowas wie

case DOWN:
   currentDirection = Direction::Up;
   break;

[Th69]

Bzgl. Namensgebung würde ich statt "breadth" doch eher "width" verwenden (ist zwar inhaltlich nicht falsch, aber ich kenne keine API, die diesen Ausdruck verwendet

[Dobi]

rapso schrieb:

Dobi schrieb:

...sowas wie position += Direction::Up; sagen

stimmt, sowas ist besser also meine magic numbers, also sowas wie

case DOWN:
   currentDirection = Direction::Up;
   break;

Dazu vielleicht noch etwas, um das Geswitche los zu werden:

std::map<Datentyp_den_getch_returned, Direction> keyToDirection;

Da dann die view Zuordnungen rein und man kann sowas machen:

currentDirection = keyToDirection[_getch()];

Dass bei unbekannten Tasten die Map wächst (Direction sollte ja eh nen default ctor mit 0,0 haben.) kann man entweder ignorieren oder halt vorher mit

auto key = _getch();
if (keyToDirection.find(key) != std::end(keyToDirection))

nachschaun.

[Dobi]

@Zero07: Falls du Lust drauf hast: Zufällig hab ich vor Kurzem auch ne Snake/Tron-Variante geschrieben, hauptsächlich um mal SFML kennen zu lernen.
http://daiw.de/share/Dron.zip Source liegt bei.

[Zero07]

rapso schrieb:

wieso ist eigentlich COORD gross geschrieben? ist das ein macro oder sowas? deine "Snake" klasse folgt anderen regeln.

Das ist doch so in windows.h definiert.

Dobi schrieb:

Dazu vielleicht noch etwas, um das Geswitche los zu werden:

std::map<Datentyp_den_getch_returned, Direction> keyToDirection;

Die Idee an sich ist gut, aber macht es in dem Fall Sinn? Angenommen die "currentDirection" ist rechts, dann kommt als nachfolgende Richung ja nur hoch oder runter in Frage. Diese Überprüfung ist doch mit einem switch in dem Fal einfacher oder irre ich mich da?!

Dobi schrieb:

@Zero07: Falls du Lust drauf hast: Zufällig hab ich vor Kurzem auch ne Snake/Tron-Variante geschrieben, hauptsächlich um mal SFML kennen zu lernen.
http://daiw.de/share/Dron.zip Source liegt bei.

Ich schau es mir bei Gelegenheit mal an

[Dobi]

Zero07 schrieb:

Dobi schrieb:

Dazu vielleicht noch etwas, um das Geswitche los zu werden:

std::map<Datentyp_den_getch_returned, Direction> keyToDirection;

Die Idee an sich ist gut, aber macht es in dem Fall Sinn? Angenommen die "currentDirection" ist rechts, dann kommt als nachfolgende Richung ja nur hoch oder runter in Frage. Diese Überprüfung ist doch mit einem switch in dem Fal einfacher oder irre ich mich da?!

Wenn man schon nach rechts fährt, ist rechts irgendwie schon ne gültige Richtung für den nächsten Zeitschritt.
Wenn jemand versucht, rückwärts zu fahren, kann man das entweder erlauben (und ihn sterben lassen) oder die Eingabe verwerfen, was ja dank schön überladenen Operatoren in der Vektorklasse einfach geht:

if (nextDirection - currentDirection == Direction::none)
    nextDirection = currentDirection;

(So hab ichs in meiner Dron\src\Dron\PlayerControllers\KeyController.cpp Zeile 52 gemacht.)
Was man von beidem dann nimmt, hängt halt davon ab, welches Gameplay man wünscht.
Ich mag es, solche Ablauflogik in maps zu schieben anstatt sie hart mit switchs oder ifs im code zu haben, weil ich persönlich den Code dann übersichtlicher finde. Ein weiter (und nicht nur subjektiver ;)) Vorteil ist vielleicht auch der, dass wenn du später mal von 2d auf 3d gehen (oder nur Diagonalen hinzufügen) willst, du nichts weiter tun musst, als nur neue keys+directions in die map zu schieben. Das könnte ja sogar von außen passieren (configurierbare Eingabetasten, Joypad, sonstwas). Der Rest funktioniert dann automatisch.

[hustbaer]

Vorweg: ich finde nicht dass das zu viel Code für so ein simples Spiel ist - so simpel ist Snake nämlich auch wieder nicht. Kommt natürlich immer drauf an mit was man es vergleicht, aber verglichen mit z.B. Pong oder Zahlen-Raten ist Snake hoch-komplex
(Und so viel Code ist es auch wieder nicht, sind ja bloss ein paar Zeilen)

Also mich stört noch irgendwie die main Funktion. Die muss noch viel zu viel über das Spiel wissen, um es richtig "bedienen" zu können.

Die einfachste Variante wäre den ganzen Loop einfach mit in das Spiel zu packen.

Im derzeitigen Zustand macht es nichtmal Sinn dass main ein eigenes Snake Objekt instanziert. Es gibt ja sowieso nur eine Snake Klasse, und es werden auch keine Parameter gesetzt bevor das Spiel losgeht.
In dem Fall würde es also reichen der Snake Klasse eine statische play Funktion zu verpassen, die dann 1:1 den Code deiner derzeitigen main enthält. Bis auf das sowieso fragwürdige system("pause") .

Ein paar andere Dinge - und bitte ignorier' diese einfach wenn es nicht die Art von Tip ist die du suchst. Einige davon sind auch für ein Projekt/Spiel dieser Grössenordnung vollkommen irrelevant, werden aber bei grösseren Projekten schnell ... naja sagen wir mal nicht ganz uninteressant.

* Du hast ein paar Funktionen in deiner Snake Klasse die eigentlich nichts mit Snake zu tun haben. Was man auch schön daran sieht, dass sie keinerlei Member der Snake-Klasse verwenden, keine privaten Memberfunktionen aufrufen etc.
Ein klarer Fall sind z.B. gotoxy und print .
Dann ist da noch printBorder . printBorder greift zwar auf height und breadth zu, aber die könnte man genau so gut als Parameter übergeben.
Ich würde daraus freie Funktionen machen. u.U. in einem anonymen Namespace definiert, in dem .cpp File wo die Klasse Snake implementiert wird.

• getFieldSize_x und getFieldSize_y machen mMn. keinen Sinn, ich würde daraus gleich Konstanten machen. Oder ganz auf die zwei verschiedenen Werte (1x mit Rand und 1x ohne) verzichten - du brauchst die Werte ohne Rand an genau einer Stelle, da kannst du mMn. genau so gut das "-2" direkt hinschreiben. Oder auch die Spezialbehandlung für den Rand ganz rausnehmen - du prüfst ja sowieso vorher ob das Feld frei ist, und SO viel Rand gibt es auch nicht dass da ein ernsthaftes Performance-Problem entstehen würde.

* Du verwendest die gleiche Benamsung für Parameter, lokale Variablen und Membervariablen. Gleiche Benamsung für Parameter und lokale Variablen ist mMn. vollkommen OK, aber bei Membervariablen finde ich es sehr irritierend. Viele Projekte verwenden hier als Prefix einfach "m_". Es gibt noch andere Konventionen, aber soweit ich beurteilen kann ist "m_" die am stärksten verbreitete, und ich verwende es auch selbst.

* Dein Snake Spiel macht im Moment zumindest zwei Dinge, die mMn. nichts mit dem eigentlichen Spiel zu tun haben: es kümmert sich darum wie und wo der Input herkommt, und wie der Output ausgegeben wird (wie die Grafik angezeigt wird). Ersteres ist einfach zu ändern, und auch in grösseren Projekten interessant. De Grafikausgabe ist eine andere Sachen. In grösseren Projekten ist die auch oft eng mit dem Gamecode verwoben, einfach aus Performance-Gründen. Wenn man es sich allerdings leisten kann hier eine bessere Trennung zu machen, halte ich es für eine gute Idee es auch zu tun.

* Wurde schon erwähnt, aber egal: die Benamsung deiner Funktionen/Variablen/... ist nicht optimal. Den "Stilbruch" bei getFieldSize_x und getFieldSize_y hat rapso schon erwähnt. "collision" finde ich auch nicht optimal. "isHeadCollision" oder "hasSnakeCrashed" fände ich sprechender.

* Die verwendeten Typen. Wieso short points ? Ich würde dem Score nen ganzen int spendieren. Bzw. wenn es keine Scores < 0 geben kann dann nen unsigned int . Weil's wurscht ist, du hast davon genau eine Variable, die paar Bytes kannst du dir leisten.
Auf der anderen Seite - wieso short field[][] ? Du hast so wenig verschiedene Zustände, da reicht ein char/unsigned char . COORD finde ich auch komisch. OK, es ist die von Windows "vorgegebene" Struktur um Positionen innerhalb der Konsole zu speichern. Ich würde trotzdem entweder einfach POINT verwenden, oder gleich eine eigene Struktur definieren.

[hustbaer]

Und weil's mir gerade aufgefallen ist ... die "food" Geschichte.

Zero07 schrieb:

void Snake::setRandomFood() { 
     bool freePos = false; 

     while(!freePos) { 
         food.X = rand() % (getFieldSize_x())+1; 
         food.Y = rand() % (getFieldSize_y())+1; 

         if(field[food.Y][food.X] != 1) { 
             freePos = true; 
         } 

     } 
     gotoxy(food.X, food.Y); 
     cout << iconFood; 

     field[food.Y][food.X] = 2; 
}

Die freePos Variable ist etwas, was ich gar nicht mag: eine Variable die nur dazu dient einen Loop zu beenden. In diesem Fall schnell zu sehen. Mach das in einer komplizierteren Schleife die über 20+ Zeilen geht, und man sieht es garantiert nicht mehr auf den ersten Blick.

Lässt sich genau so gut so schreiben:

for (;;) { 
         food.X = rand() % (getFieldSize_x())+1; 
         food.Y = rand() % (getFieldSize_y())+1; 

         if(field[food.Y][food.X] != 1)
             break; // break ist nicht böse
     }

Und in diesem Fall, da die Abbruchbedingung die letzte Anweisung in der Schleife ist, sogar noch einfacher mit do-while:

do { 
         food.X = rand() % (getFieldSize_x())+1; 
         food.Y = rand() % (getFieldSize_y())+1; 
     }
     while (field[food.Y][food.X] == 1); // bzw. gleich != 0   -- mit == 1 bekommst du z.B. ein Problem sobald du mehr als nur ein "food" erlaubst

Dann die Ausgabe des "Food Icons" in setRandomFood - das ist duplizierter Code, genau dafür hast du ja die print Funktion geschrieben.

Und wozu ist die "food" Membervariable gut? Ich finde die bloss verwirrend. Du verwendest die an genau zwei Stellen, nämlich einmal in setRandomFood und dann nochmal in initializeField . Wobei noch dazu eine unnötige Abhängigkeit entsteht, nämlich dass setRandomFood vor initializeField aufgerufen wird.
Alles in allem würde ich das eher so schreiben:

void Snake::setRandomFood() { 
     COORD pos; // das Member "food" brauchen wir nimmer
     do
         pos = getRandomPosition(); // neue Funktion
     while (!isTileFree(pos)); // noch eine neue Funktion

     print(pos, iconFood);
     tileAt(pos) = 2; // nächste neue Funktion
}

short Snake::tileAt(COORD const& pos) const {
    return field[pos.x][pos.y];
}

short& Snake::tileAt(COORD const& pos) {
    return field[pos.x][pos.y];
}

COORD Snake::getRandomPosition() const {
    COORD const pos = { rand() % (breadth - 2) + 1, rand() % (height - 2) + 1 };
    return pos;

    // bzw. C++11:
    return { rand() % (breadth - 2) + 1, rand() % (height - 2) + 1 };
}

bool Snake::isTileFree(COORD const& pos) const {
    return tileAt(pos) == 0;
}

void Snake::initializeField() { 
    COORD pos;
    for (pos.x = 0; pos.x < height; pos.x++) { 
        for (pos.y = 0; pos.y < breadth; pos.y++) { 
            bool const isBorder = pos.x == 0
                || pos.x == height - 1
                || pos.y == 0
                || pos.y == breadth - 1;

            tileAt(pos) = isBorder ? -1 : 0;
        } 
    } 
    setRandomFood();
}

[hustbaer]

Bzw. so wäre es vielleicht noch "schöner"...:

COORD Snake::getFreePosition() const { 
    for (;;) {
        COORD const pos = getRandomPosition();
        if (isTileFree(pos))
            return pos;
    }
}

void Snake::setRandomFood() { 
    COORD const pos = getFreePosition();
    tileAt(pos) = 2;
    print(pos, iconFood); // ist zwar genaugenommen egal, aber jedes mal
                          // wenn ich print() vor dem Update des Feldes sehe kommt es mir falsch platziert vor.
}