Using constants

Rather than using #define and creating a bunch of macros like this:

#define PLANEHEIGHT 20 #define PLANEWIDTH 50 #define INITIALSNAKELENGTH 3 #define FPS 5 

You should use const instead, like this:

const int PLANE_HEIGHT = 20; const int PLANE_WIDTH = 50; const int INITIAL_SNAKE_LENGTH = 3; const int FRAMES_PER_SECOND = 5; 

Using constants rather than macros adds readability to your code, and it has all the properties of a normal variable, e.g, a size, a type, linkage, etcetera. A macro has none of those.

---

Refactoring SnakeSegment

I'd also define your SnakeSegment structure as a class instead, as seen below.

class SnakeSegment { public: SnakeSegment() {}; SnakeSegment( unsigned short x1,unsigned short y1 ) { x = x1; y = y1; } unsigned short x; unsigned short y; }; 

But that's not all we can do, we can also remove the empty constructor, as it doesn't make much sense to have it. Our SnakeSegment class then becomes this:

class SnakeSegment { public: SnakeSegment( unsigned short x1,unsigned short y1 ) { x = x1; y = y1; } unsigned short x; unsigned short y; }; 

But that's still not all. There's a better way of initializing x and y in the constructor as well. Here's how the constructor would look after that:

SnakeSegment(unsigned short x, unsigned short y): x(x), y(y) {} 

There's no need to worry about the arguments of the constructor having the same names as the fields, the x in x(x) and the y in y(y) will evaluate as the constructor arguments, rather than the fields.

In short, the SnakeSegment class, after a few other minor changes, the SnakeSegment class will become this:

class SnakeSegment { public: unsigned short x; unsigned short y; SnakeSegment(unsigned short x, unsigned short y): x(x), y(y) {} }; 

It's also worth noting, if you ever want to add more methods to this class in the future, you should define two separate files for it, classname.cpp, and classname.h. classname.h should contain the class declaration and the function/constructor signatures inside it, and classname.cpp should include classname.h and implement the functions/constructors.

---

Not using rand()

rand() is a really bad way to generate random numbers, you should be using a solution more along the lines of this:

std::random_device randomDevice; std::mt19937 engine(randomDevice()); std::uniform_int_distribution<int> distribution(low, high); int result = distribution(engine); 

Do note, you will have to add this line of code to the top of your file:

#include <random> 

---

Nitpicks

Please, please do not use this "hack":

system("PAUSE"); 

It's not cross-platform, it's not readable, and it's insecure, if you need the program to pause before exiting, use something like this:

std::cin.get(); 

From looking at your code though, I think that your program should just exit, so there's no need for either of these solutions.

Finally, all these function signatures at the top of your code:

void DisplayPlane(); void NormalisePlane(); void SnakeInit(); void FoodInit(); void Move(); void DeleteLastSnakeSegment(); bool HasEatenFood(); void WipeConsole(); bool Won(); bool Lost(); 

These are a bit of code smell, and are not really necessary, they can be removed. (You may have to define these functions again above main.)

It's not too bad, but there are still a few more improvements to make.

Avoid using global data

Defining a program in terms of global variables is a rather crude way of doing things. Globals provide an easy scape hatch when you need to share data between two pieces of code, but that won't scale well. You're fine now with a single source file, but it would easily become chaos if the projects grows. When you have global data, any piece of code can read/write this data, which creates strong coupling between functions/classes and makes it a lot harder to reason about the program.

So how can you fix this? There are two basic ways: If you only have a couple variables to keep around, declare them at the top level function (in this case main) and pass them as parameters. But your game already has a considerable amount of data to handle, so a better approach would be the group that data into another class, perhaps a SnakeGame class. Naturally, then make the functions members of SnakeGame.

You should aim at having your main function look something like this:

int main() { SnakeGame game; game.run(); // only returns when the user quits. } 

Now that would be a neat solution ;)

Other stylistic bits and small fixes

• Avoid writing excessively long lines, such as this one:

  for( unsigned short i = 0; i < PLANEWIDTH + 2; i++ ) printf( "%c",'/' ); printf( "\n" ); //lower bound 

  No reason to pack all that in a single line. Rule of thumb is: one statement per line. This facilitates reading the code. Also, don't assume that everyone reading your code will be doing so on a 20+ inches wide-screen.

• In the Move() function, all paths in the if/else-if chain look very similar, the only change being how the segment gets incremented. Don't repeat yourself. Introduce another small helper function that does the common work.

• You create a temporary food variable in HasEatenFood(), but that variable is not really necessary. You can just compare the segment with foodX and foodY.

• You're not using the best data structure for your snake segments. When removing an element, you always erase the first one, so the array gets shifted by 1. In a simple game like this, that's of no consequence, but doing that frequent array shifting in an array of heavy weight objects could bring your program to a crawl. std::vector must maintain the data sequentially laid out in memory, so erasing from anywhere but the end (pop_back) is a linear time operation. std::deque on the other hand, has no such requirement and is optimized for insertion and removal on both ends (deque is an acronym for double-ended queue). And BTW, instead of repeating m_Snake.size() - 1, just use the front() and back() methods instead, it is a lot cleaner that way.

• Another thing Ethan didn't mention about using const over #define is that constants respect scope (the main advantage IMO), so you can, for instance, make them class members (your constants could be static members of the SnakeGame). Another note, more of personal style but there is some level of agreement about it, is that ALL_UPPERCASE should be reserved for macros (AKA #defines), precisely to make them stand out from the rest (we want to keep an eye out for those bad boys ;)). So a alternate common notation for constants is MultiCase, e.g.: const int InitialSnakeLength = ...

• std::cout and the standard C++ streams are more robust than printf and the C IO library. Format strings (those annotated %s that you pass in the string) tend to be more readable and there is some value in separating the data from the presentation, but unfortunately, they are a C mechanism that is incompatible with C++'s object model. So you cannot, for instance, pass a C++ class type to one of those functions (don't even think about passing it a std::string!). Format strings are also not type safe, since you wave compile-time type inference and rely on manually matching the type of each parameter with the %s in the string. In the end, that's a recipe for disaster and these issues outweigh any gains in readability. For the record, the Boost libraries provide similar formatting functionality but with the type safety of C++. I don't think it would be worth adding it to such a small project, so just give the C++ streams a try instead.

I took your program and improved it so it won't flicker, and it works well. Basically I just used the function SetConsoleCursorPosition of windows.h to print only the new snake segment, and erase only the last snake segment, instead of printing the whole plane again.

COORD pos; pos.X = segment.x + 1; //segment is the head of the snake pos.Y = segment.y + 1; SetConsoleCursorPosition(out, pos); printf("O");