Revision 3f84d701-738d-4e06-bd3e-e32b45d25bf8 - Code Review Stack Exchange

Nice first question.

I will not cover anything that B. Wolf has already covered. I'm also sorry that this could not be more comprehensive, I have not reviewed all the functions.

The code is nicely indented, some of the variable names and function names are clear. The variables `t and S` in `main()`,
`W and i` in `renderMenu()` and `S` in `reachingFood()` deserve more descriptive names.

**Inconsistent Coding Style**

The naming of functions and variables are inconsistent, they should all use one convention. Use
either camelCase or some other convention, but consistent is much better. You are stuck with
the names of the functions from ncurses, but all other functions and variables should be consistent.
`usDelay`, `secsElapsed`, `gamewin`, `isOccupied`, `collided` and `highlight` are examples of
inconsist coding conventions.

**Utilize Good Programming Principles**

The code is missing out on some basic principals that might help such as [Reducing Coupling][1], [SRP][2]
and [KISS][3]. The functions `main()`, `moveSnake()` and possibly `newSnake()` are too long and overly complex. `main()` and
`moveSnake()` should be broken up into smaller functions. It is very *rare* for a function to be 100 or more
lines of code.

**Avoid Tight Coupling**

Avoiding tight coupling has been a programming principle that even dates to before most of
the object oriented languages. The function `wmvaddch()` is defined in main.c but it is used
in snake.c, this is a form of tight coupling and should be avoided. Since `wmvaddch()` is
only used in snake.c it should be moved to snake.c and it doesn't need to in snake.h.

**Use the Single Responsibility Principle to Reduce Complexity of the Functions**

I've been told by some managers that any function over 10 lines is too long. In
any case functions should really fit on one screen and anything longer than 30 (an arbitrary view point) lines is too complex.
The main() function should generally be a set of functions to perform whatever setup is necessary,
execute the program function and perform whatever cleanup is necessary.

The main() function can be broken up into the following functions:

setupCursesForSnake().
setupDisplay().
setupUserLevel().
playSnake().
cleanup().

Example of simiplified function:

 playSnake(Snake *S, int yMax, int xMax)
 {
 bool collided = false;
 clock_t t;
 double secsElapsed;
 int usElapsed;
 Window *scoreWin = newwin(4,14,yMax-3,xMax-14);
 refresh();
 updateScore(scoreWin,S,highlight+1);
 
 while(!collided){
 t = clock();
 flushinp();
 choice = wgetch(gamewin->W);
 t = clock() - t;
 secsElapsed = ((double)t)/CLOCKS_PER_SEC; // seconds
 usElapsed = (int)(secsElapsed*1000000); // microseconds
 if(choice == ERR) choice = S->lastDir;
 else usleep(usDelay-usElapsed);
 
 collided = moveSnake(gamewin,S,choice);
 renderSnake(gamewin->W,S);;
 updateScore(scoreWin,S,highlight+1);
 }
 
 cbreak();
 freeGW(gamewin);
 clear();
 refresh();
 updateScore(scoreWin,S,highlight+1);
 Window *endGameWin = newwin(menuHeight,menuWidth,(yMax-menuHeight)/2,(xMax-menuWidth)/2);
 keypad(endGameWin,TRUE);
 refresh();
 }

The previous example would simplify a "Play Another Game" function.

In the function `moveSnake()` The code within the `if clause` and the `else clause` should be
functions to simplify the `moveSnake()` function.

**MAGIC Numbers**

Because the main function is so long I initially missed the `return -1;` statement. One suggestion
about this statement is rather than using -1, use the symbolic constant EXIT_FAILURE that
is defined in `<stdlib.h>`. While it is not completely necessary, to improve the consistency of
the code use return EXIT_SUCCESS at the end of main(). These will make your code clearer
and the constants are available on all platforms, the can also be used in C++. The exit(1) in
`moveSnake()` could be exit(EXIT_FAILURE).

In the following code the numbers 0, 1, 2, 5, 3, 1, 500000, 300000 and 100000 are what some
professional programmers call magic numbers.

 switch(highlight){
 case 0:
 usDelay = 500000;
 halfdelay(5);
 break;
 case 1:
 usDelay = 300000;
 halfdelay(3);
 break;
 case 2:
 usDelay = 100000;
 halfdelay(1);
 break;
 }

Use symbolic constants for these numbers to make the code easier to read and debug, and in many cases code. 

 #define EXPERT_DELAY 100000
 #define INTERMEDIATE_DELAY 300000
 #define BEGINNER_DELAY 500000

A better way to handle 0, 1 and 2 might be to use an enumeration type (enum).

 typedef enum {BEGINNER, INTERMEDIATE, EXPERT} PlayerLevel;
 PlayerLevel playerLevel = INTERMEDIATE;

 switch(highlight){
 case BEGINNER:
 usDelay = BEGINNER_DELAY;
 halfdelay(BEGINNER_HALF_DELAY);
 break;
 case INTERMEDIATE:
 usDelay = INTERMEDIATE_DELAY;
 halfdelay(INTERMEDIATE_HALF_DELAY);
 break;
 case EXPERT:
 usDelay = EXPERT_DELAY;
 halfdelay(EXPERT_HALF_DELAY);
 break;
 }

**Avoid Implicit Casting**
The variable relSize is type float, the right hand side of the lines:

 int boundY = yMax/relSize;
 int boundX = xMax/relSize;


will result in floats and that code is truncating the values of the right hand side. In some
instances this might result in the wrong value being assigned to boundY and boundX. The standard
C include file includes the functions [round][4](), ceil() and floor() and are more appropriate that the
simple truncation here.

Using type double rather than type float will be more accurate.

In any case it is much better to avoid implicit casts and make them explicit:


 int boundY = (int) (yMax/relSize);
 int boundX = (int) (xMax/relSize);

**Use exit() Very Carefully**
The use of the exit() function may bypass cleanup functions. In C++ you can avoid the exit()
function by throwing and catching exceptions, in C there is an error handling capability using
`setjmp()` and `longjmp()`. Generally setjmp() would be used in main() and longjmp would be
used where error conditions happening. [This stackoverflow.com question discusses setjmp() and longjmp().][5]

The use of exit() should be avoided in programs such as operating systems that are never supposed
to end.

The printf() preceeding the call to exit() would be better as an `fprintf(stderr, ERROR_MESSAGE);`.
The printf() function prints to stdout and may not appear in all cases, stderr should appear in
all cases.


 [1]: https://en.wikipedia.org/wiki/Coupling_(computer_programming)
 [2]: https://en.wikipedia.org/wiki/Single_responsibility_principle
 [3]: https://en.wikipedia.org/wiki/KISS_principle
 [4]: http://www.codecogs.com/library/computing/c/math.h/round.php
 [5]: http://stackoverflow.com/questions/14685406/practical-usage-of-setjmp-and-longjmp-in-c