Code Review/Share! My first C++ game (Snake)

[CajunCoder]

Hi guys.

I thought I would share this for two reasons:

1) It's my first c++ project, so I'd like to know if I'm doing things right! In particular:
> Is my code well structured and clearly written?
> Am I following C++ conventions correctly?
> Am I misusing anything? Handling pointers okay? Threads okay?

I'm looking for constructive criticism, basically!

2) Provided the code isn't terrible, I thought it might make for a good base for other beginners who might want to make their own ASCII games.

For simplicity's sake, and because I like to avoid the pain of header files wherever possible and this is a really simple/small program, all code is in one file.


You will need a copy of mingw.thread.h in the same folder as this file if you are compiling with gcc/g++. You can find it here -> https://github.com/meganz/mingw-std-threads

Alternatively, you can remove the threaded call to beepJingle() in logic().

Requires C++11.

//use g++ -std=c++11 -static filename.cpp to include included libraries in exe (GCC/G++)
#include <iostream>
#include <string>
#include <stdio.h>
#include <cstdlib>
#include <conio.h>
#include <windows.h>
#include <vector>
#include <chrono>
#include "mingw.thread.h"
#include <cmath>
// copy of "mingw.thread.h" needed if you are compiling in g++.
/* If you wish to run this code without that file, remove the threaded
call to beepJingle() in logic() */
// also; geez. How on earth does gcc/g++ not support threading in its current state?

// Windows Dependencies: Console Text Attributes, Beep, Sleep, Cursor Position stuff, system("cls")?
/* Given the limited scope of this project, I'm not interested in porting it, but
you're welcome to do so if you so desire! */

using namespace std;
HANDLE hConsole = GetStdHandle(STD_OUTPUT_HANDLE);
bool running = true;
const int sizeX = 11;
const int sizeY = 11;
char raster[sizeX][sizeY];
int foodx;
int foody;
char map[sizeX*sizeY] = {//No reason not to hard code this
	'#','#','#','#','#','#','#','#','#','#','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#',' ',' ',' ',' ',' ',' ',' ',' ',' ','#',
	'#','#','#','#','#','#','#','#','#','#','#'
	};

class SnakeBit{
	
	private:
	int x;
	int y;
	int lastx; //I originally had these two variables public
	int lasty; //and accessed them directly, but figured that's bad?
	
	public:
	
	SnakeBit(int x,int y){
		this->x=x;
		this->y=y;
		lastx=x;
		lasty=y;
	}
	void setPosition(int x,int y){
		lastx = this->x;
		lasty = this->y;
		this->x = x;
		this->y = y;

	}
	int getx(){
		return x;
	}
	int gety(){
		return y;
	}
	int getlastx(){
		return lastx;
	}
	int getlasty(){
		return lasty;
	}
};

vector<SnakeBit> snake;
enum direction{STOP=0, LEFT, RIGHT, UP, DOWN};
direction dir;
SnakeBit *head;

void gotoXY(int x, int y) 
{ //Oh hey, I guess this is windows specific too
    static COORD CursorPosition;
    CursorPosition.X = x;
    CursorPosition.Y = y;
    SetConsoleCursorPosition(hConsole,CursorPosition); 
}

void beepJingle(){
	Beep(1000,120); //windows specific
	Beep(1261,120);
	Beep(1324,120);
}

void newFood() {
	foodx = (rand()%(sizeX-2))+1;
	foody = (rand()%(sizeY-2))+1;
}

void addSnakeBit() {
	SnakeBit lastBit = snake[snake.size()-1];
	snake.push_back(SnakeBit(lastBit.getlastx(),lastBit.getlasty())); //this reallocs memory unless we use vector.reserve(size);
	//we need to reassign our head pointer
	//we don't need to delete head because its reference is already deleted by vector (right?)
	head = &snake[0];
}

void setUp(){
	newFood();
	snake.clear();
	snake.push_back(SnakeBit(5,5));
	dir = STOP;
	head = &snake[0];
}

void draw() {
	gotoXY(0,0);
	SetConsoleTextAttribute(hConsole, 8);
	cout << "_____ASCII_SNAKE_____" << endl;
	//initial raster
	for(int gy=0; gy<sizeY; gy++)  {
		for(int gx=0; gx<sizeX; gx++) {
			raster[gx][gy] = map[gx+(gy*sizeX)];
		}
	}
	
	//food to raster
	raster[foodx][foody] = 'F';
	
	//player to raster
	raster[head->getx()][head->gety()] = '@';
	for(unsigned int i = 1; i < snake.size(); i++){
		raster[snake[i].getx()][snake[i].gety()] = 'o';
	}
	
	//draw raster
	for(int y=0; y<sizeY; y++)  {
		for(int x=0; x<sizeX; x++) {
			char c = raster[x][y];
			if(c=='#') {SetConsoleTextAttribute(hConsole, 2);}
			if(c=='F') {SetConsoleTextAttribute(hConsole, 12);}
			if(c=='@') {SetConsoleTextAttribute(hConsole, 14);}
			if(c=='o') {SetConsoleTextAttribute(hConsole, 14);}
			if(c==' ') {SetConsoleTextAttribute(hConsole, 10);}
			cout << c;
			cout << ' ';
		}
		cout << endl;
	}	
	SetConsoleTextAttribute(hConsole, 8);
	cout << "Score: " << snake.size()-1 << endl;
	cout << "WASD - Move" << endl;
	cout << "X - Quit." << endl;
	
}

void getInput() {
	if(_kbhit()){
		char c = _getch();
		if(c == 'x') {running = false;}
		if(c == 'w' && dir!=DOWN) {dir = UP;}
		if(c == 'a' && dir!=RIGHT) {dir = LEFT;}
		if(c == 's' && dir!=UP) {dir = DOWN;}
		if(c == 'd' && dir!=LEFT) {dir = RIGHT;}
	}
}

void logic() {

	//movement
	switch(dir){//head
		case UP:
			head->setPosition(head->getx(),(head->gety())-1);
			break;
		case DOWN:
			head->setPosition(head->getx(),(head->gety())+1);
			break;
		case LEFT:
			head->setPosition((head->getx())-1,head->gety());
			break;
		case RIGHT:
			head->setPosition((head->getx())+1,head->gety());
			break;
		default: break;
	}
	if(snake.size()>1){//tail bits
		for(unsigned int i = 1; i < snake.size(); i++) {
			
			SnakeBit lastLink = snake[i-1];
			snake[i].setPosition(snake[i-1].getlastx(),snake[i-1].getlasty());
		}
	}
	//check death
	if(raster[head->getx()][head->gety()] == '#' || raster[head->getx()][head->gety()] == 'o') {
		running = false;
	}
	//eat food
	if(raster[head->getx()][head->gety()] == 'F') {
		thread t{beepJingle};
		t.detach();
		addSnakeBit();
		newFood();
	}
	
}

int main(){
	bool playAgain = true;
	char input;
	while(playAgain) {
		setUp();
		system("cls"); //windows specific?
		while(running){
			
			getInput();
			logic();
			draw();
			//std::this_thread::sleep_for(std::chrono::milliseconds(100)); //<-didn't know how to use this, used Windows function instead.
			Sleep(400); //windows specific
		}
		cout << "* GAME OVER *" << endl;	
		cout << "Play again? y/n" << endl;
		cin >> input;
		if(input=='y' || input=='Y'){
			playAgain=true;
			running=true;
		}else{
			playAgain=false;
		}
	}
	SetConsoleTextAttribute(hConsole, 10); //windows specific
	return 0;
}

[coder777]

For threading #include <thread> . Recent versions of gcc will support that.

I would suggest that you omit lastx/y since they are not necessary. Also omit head since it is always snake[0]. Remove direction dir; on line 82 as well, since this could be local:

direction getInput() {
...
}
...
void logic(direction dir) { // Note: 'logic' is not a good name, use move_snake would be more descriptive
}
...
			logic(getInput());

Further I suggest that you make x/y public since they are modified from outside the class and not use inside the class.

Moving the tail could be simplified:

void logic(direction dir) { // move_snake() seems to be a better name

if(snake.empty()) // This ensures that we have a snake
;
else
{
SnakeBit old_end_of_tail = snake.back(); // this is for growing

		for(unsigned int i = 1; i < snake.size(); i++) { // Note: because of i = 1 we don't need to check
			
			snake[i] = snake[i-1]; // Simple assignment suffice
		}

SnakeBit *head = snake[0]; // For simplification sake
	//move the head after the tail!
	//movement
	switch(dir){//head
		case UP:
			head->setPosition(head->getx(),(head->gety())-1);
			break;
		case DOWN:
			head->setPosition(head->getx(),(head->gety())+1);
			break;
		case LEFT:
			head->setPosition((head->getx())-1,head->gety());
			break;
		case RIGHT:
			head->setPosition((head->getx())+1,head->gety());
			break;
		default: break;
	}


	//check death
	if(raster[head->getx()][head->gety()] == '#' || raster[head->getx()][head->gety()] == 'o') {
		running = false;
	}
	//eat food
	if(raster[head->getx()][head->gety()] == 'F') {
		thread t{beepJingle};
		t.detach();
// Note: growing is now simple:
snake.push_back(old_end_of_tail);
		newFood();
	}
}
	
}

[TheIdeasMan]

Hi,

use g++ -std=c++11 -static filename.cpp

When compiling always set the warning level as high as possible; warnings are your friend:

g++ -std=c++11 -Wall -Wextra -pedantic-errors
There are some other warnings that are handy despite the seemingly all singing all dancing options above:
http://www.cplusplus.com/forum/general/183731/#msg899203

Try to get the latest version of the compiler, you can use the latest standard conformance. The current version of g++ is 6.2.1

Avoid using conio.h it's not standard. It's more complex, but have a look at ncurses.

It's very good practise not to have global variables.

Put your class declaration into a header file with the same name as the class. I like to use the .hpp extension - to humans it means a c++ header file.

Put the code for the class member functions into a cpp file with the same name as the class.

One doesn't have to always routinely make a get and set function for each member variable. You don't seem to ever call the set functions. One only needs set functions if the value of a member variable needs to change after it has been constructed. Look into using member initialisation lists in the constructor. And it is better to think clearly about what the interface to your class (the public functions) is going to be - what does the class do; what does the user of a class need to be able to do?

Make life easier for your self by giving the parameters to your constructor different names than the member variables - no need for the use of this->. I like to name my parameters with "Arg" appended, as in xArg and yArg

Yes public or protected variables are bad.

Line 81: The first value in an enum defaults to zero, so no need to be explicit there.

line 100: if you are going to use rand() , then you need to seed the generator with srand


Functions should do 1 conceptual thing, so check death and eat food should be separate functions.

Instead of if(input=='y' || input=='Y'){ make use of either toupper or tolower to convert the char into upper case, say. Then you could have this: if( input=='Y'){

Try to avoid having using namespace std; , believe me it's best and easiest int the end to just put std:: before each std namespace thing. The reason is that one day you will be bitten by a name conflict and wonder why :+) , so IMO it's best to learn that lesson early. There is plenty written about why it is bad. If you look at code by experts such as JLBorges, they always use std::

I like to avoid naming variables after something that exists in the std namespace. Like map for example. I know that using std:: avoids that problem, I guess it's my personal preference.

It's also a good idea to put all of your own code into it's own namespace/s.


Good Luck !!

[jlb]

Requires C++11.

Then why are you using those C standard headers instead of the equivalent C++ standard headers, ie <stdlib.h> should be <cstdlib>.

I'm not a Windows programmer but shouldn't <windows.h> be the first #include?