Skip to content

Instantly share code, notes, and snippets.

@luizperes
Last active September 3, 2016 06:39
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save luizperes/4aa66eb946676e70684aa895ab3e565e to your computer and use it in GitHub Desktop.
Save luizperes/4aa66eb946676e70684aa895ab3e565e to your computer and use it in GitHub Desktop.
// Your project is amazing. Thanks for it. Here goes my considerations:
/* The first thing that called my attention was that you had 4 warnings on your project.
* I want you to understand that warnings are in fact possible future runtime errors, thus
* we need to try to solve them before shipping a product.
*
* After that, one other thing that I saw as I opened your project was that you did not quite
* choose good names for your different files. I am saying that because they are arranged as
* function names, for example: Start_Game.c should call main.c, or only game.c, as the names
* should be easy to be identified by any programmer. Other example would be the 'Print_Matrix.c',
* I imagine that this file should call 'MatrixHelper.c' or something else, but 'Print_Matrix' is
* a function's name rather than a class' one. Imagine that files should be treated as classes, even though
* C does not have the abstraction of them. Also, pattern is one of our most important skills, therefore,
* you should keep either 'CamelCase' or 'Camel_Case'. You have some files that are not compliant with the
* pattern of others
*/
/*
* This file is one of your best ones. You kept it very simple and simplicity is gold.
* One point here is that you have a 10x10 matrix and you are using the number 99 for cheating.
* In a matrix 10x10, we have 0..9 rows and 0..9 columns, thus we can never 'hit' a ship if it ends up there.
* It happened with me when I was testing, haha.
* You could've used any other number greater than that, though.
* Don't use chars for control :) use booleans(aka char) or only the values(0 and 1).
*
* This fseek to the end of the buffer is beautiful! Congrats!
*/
#include "GameLoop.h"
void startTheGame() {
char shallExitGame = 'N';
unsigned int userI, userJ, oneNumber;
unsigned int n;
printf("\n");
printf("====== LET'S START SHOOTING! ======\n");
printf("= (111 - Exit, 99 - See the ships) =\n");
printf("===================================\n\n");
while (shallExitGame == 'N') {
do {
printf("Line and Column as one number: ");
n = scanf("%d",&oneNumber);
if (n !=1) {
fseek(stdin,0,SEEK_END);
printf("Sorry! Did not capture your enter! Please trye again!\n");
}
} while (n != 1);
if (oneNumber == 111) {
shallExitGame = 'Y';
continue;
}
else if (oneNumber == 99) {
printf("\n");
printMatrix(0);
printf("\n\n");
continue;
}
userI = (oneNumber - oneNumber % 10)/10;
userJ = oneNumber % 10;
hitCheck(userI,userJ);
}
}
/*
* I don't know how this program is working, haha
* Your file calls "HitCheck.h", but you are calling "hitCheck.h".
* This must be one of XCode's tricks.
* Remembering that Unix based systems have case sensitive paths ;)
* Wasn't this file supposed to call "Game_Loop.h"? Stick to one pattern.
*/
#ifndef GameLoop_h
#define GameLoop_h
#include <stdio.h>
#include "Global_Data.h"
#include "Print_Matrix.h"
#include "hitCheck.h"
void startTheGame();
#endif /* GameLoop_h */
// Please see Global_Data.h
#include "Global_Data.h"
unsigned int battelFieldMatrixXY = 10;
unsigned int battleFieldMatrix[10][10] = {};
unsigned int allShipsMatrix[20] = {11,
12,
13,
14,
21,21,
22,22,
23,23,
31,31,31,
32,32,32,
41,41,41,41};
/*
* Try to not include libraries that you don't need.
* As well as avoid as much as you can to create global variables.
* Global variables should be used for readonly purposes.
* http://c2.com/cgi/wiki?GlobalVariablesAreBad
*/
#ifndef Global_Data_Defined
#define Global_Data_Defined
#include <stdio.h>
extern unsigned int battelFieldMatrixXY;
extern unsigned int battleFieldMatrix[10][10];
extern unsigned int allShipsMatrix[20];
#endif
/*
* You should have included "HitCheck.h" oO
* If you have smaller statements, you won't need things like:
* //end (battleFieldMatrix[i][j] != 0)
*
* Use enums in your switches and returns :) They are very nice.
*/
#include "hitCheck.h"
void hitCheck(unsigned int i, unsigned int j) {
unsigned int shipHit;
if (battleFieldMatrix[i][j] != 0 && battleFieldMatrix[i][j] != 77) {
shipHit = battleFieldMatrix[i][j];
battleFieldMatrix[i][j] = 77;
switch (findShip(shipHit)) {
case 0:
printf("\n");
printf("======== TOTAL ANNIHILATION! ======\n");
printf("===== DESTROYED SHIP'S SIZE: %d ====\n\n", (shipHit-shipHit%10)/10);
break;
case 1:
printf("\n");
printf("======== YOU GOT A PIECE! =========\n\n");
break;
case 2:
printf("\n");
printf("======== ABSOLUTE VICTORY! ========\n\n");
printMatrix(1);
exit(0);
break;
}
}
else {
printf("\n");
printf("============= MISSS! ==============\n\n");
}
printMatrix(1);
}
int findShip(unsigned int shipNumber) {
static unsigned int controlSum = 77*20;
for (unsigned int i=0;i<20;i++) {
if (allShipsMatrix[i] == shipNumber) {
allShipsMatrix[i] = 77;
controlSum-=77;
if (controlSum == 0) return 2;
switch (i) {
case 19:
return 0;
default:
if (allShipsMatrix[i+1] == shipNumber ) {
return 1;
}
else {
return 0;
}
}
}
}
return 5;
}
/*
* Wasn't this file supposed to call "Hit_Check.h"?
*/
#ifndef hitCheck_h
#define hitCheck_h
#include <stdio.h>
#include <stdlib.h>
#include "Global_Data.h"
#include "Print_Matrix.h"
void hitCheck(unsigned int i, unsigned int j);
#endif /* hitCheck_h */
/*
* Try to use spaces after assignments and operations in order of better legibility, like:
* checkSumForSurraundings += battleFieldMatrix[i][leftJ];
* instead of
* checkSumForSurraundings+=battleFieldMatrix[i][leftJ];
*
* The software designers/architects also say that each function/method should be at maximum your screen's height,
* so you should probably break the function 'placeShips' down in smaller ones.
* I invite you to take a look to this code here: https://github.com/luizperes/artoo-editor/tree/master/src
*
* Instead of 'f' or 't' you should have included the stdbool.h, although they are both 1 byte long.
*
* Nested switches are not as good as breaking it down in functions. They are very hard to understand.
* Try to include blocks { } as well, you can put them anywhere in order to ease readability.
*
* It is not a good practice to leave 'ifs' as:
* if(something) break;
*
* Apple itself had a very bad security whole because of one if like that:
* if (something)
* DoIt;
* DoAnotherThing;
*
* DoAnotherThing does not belong to the if, so many programmers have the curly braces as a programming
* standard. Swift itself does not allow us to not use braces. Perhaps because of that previous failure?
*
* Always keep your statements like the one on the line below.
* if (something) {
* DoIt;
* DoAnotherThing;
* }
*
* For Switches, try to only use integers and enums instead of chars :)
*
* I like your recursivity, but perhaps a for-loop wouldn't be better?
* Remember that recursivity takes more space in memory as it forces new Stack frames
* (if the language does not have Tail Call Optimization, as Javascript, for example).
* We should use recursivity carefully. In this case is fine! :)
*
* I like that you used checksum! Not everyone knows that way of programming resolution.
*/
#include "Place_Ships.h"
// this function prototype must exist, because C reads and compile inline.
// You did not get an error because XCode solved this problem for you. If should do that for
// all other warnings.
int randomCell();
void placeShips(int size, int shipNumber) {
/*
* You got a warning here, because you did not define the prototype of the function
* 'randomCell' at the top of the file
*/
unsigned int rndCell = randomCell();
int i = (rndCell - rndCell % 10)/10;
int j = rndCell % 10;
char leftOrRight;
char upOrDown;
unsigned int checkSumForShip;
unsigned int checkSumForSurraundings;
char endSuccessfully = 'f';
int upperI, lowerI, leftJ, rightJ;
switch (rand() % 3) {
//0 - horizontal
case 0:
leftOrRight = (battelFieldMatrixXY-1)-j >= size-1 ? 'r' : 'l';
switch (leftOrRight) {
case 'r':
checkSumForShip = 0;
for (int r = j+1; r<=j+size-1;r++) {
if (battleFieldMatrix[i][r] == 0) checkSumForShip++;
}
if (checkSumForShip == size-1) {
checkSumForSurraundings = 0;
upperI = i == 0 ? 0 : i-1;
lowerI = i == battelFieldMatrixXY-1 ? battelFieldMatrixXY-1 : i+1;
rightJ = (j+size) > battelFieldMatrixXY-1 ? battelFieldMatrixXY-1 : j+size;
for (int tempJ = j+2; tempJ<=rightJ;tempJ++) {
checkSumForSurraundings +=battleFieldMatrix[upperI][tempJ] + battleFieldMatrix[lowerI][tempJ];
}
checkSumForSurraundings+=battleFieldMatrix[i][rightJ];
if (checkSumForSurraundings == 0) {
for (int tempJ = j;tempJ<j+size;tempJ++) {
battleFieldMatrix[i][tempJ] = size*10+shipNumber;
}
endSuccessfully = 't';
break;
}
}
case 'l':
checkSumForShip = 0;
for (int r = j-1; (r>=j-(size-1)) && (r >0) ;r--) {
if (battleFieldMatrix[i][r] == 0) checkSumForShip++;
}
if (checkSumForShip == size-1) {
checkSumForSurraundings = 0;
upperI = i == 0 ? 0 : i-1;
lowerI = i == battelFieldMatrixXY-1 ? battelFieldMatrixXY-1 : i+1;
leftJ = (j-size) < 0 ? 0 : j-size;
for (int tempJ = j-2; tempJ>=leftJ;tempJ--) {
checkSumForSurraundings +=battleFieldMatrix[upperI][tempJ] + battleFieldMatrix[lowerI][tempJ];
}
checkSumForSurraundings+=battleFieldMatrix[i][leftJ];
if (checkSumForSurraundings == 0) {
for (int tempJ = j;tempJ>j-size;tempJ--) {
battleFieldMatrix[i][tempJ] = size*10+shipNumber;
}
endSuccessfully = 't';
break;
}
}
}
if (endSuccessfully == 't') {
break;
}
//1 - vertical
case 1:
upOrDown = (battelFieldMatrixXY-1)-i >= size-1 ? 'd' : 'u';
switch (upOrDown) {
case 'd':
checkSumForShip = 0;
for (int r = i+1; r<=i+size-1;r++) {
if (battleFieldMatrix[r][i] == 0) checkSumForShip++;
}
if (checkSumForShip == size-1) {
checkSumForSurraundings = 0;
lowerI = (i+size) > battelFieldMatrixXY-1 ? battelFieldMatrixXY-1 : i+size;
leftJ = j == 0 ? 0: j-1;
rightJ = j == battelFieldMatrixXY-1 ? battelFieldMatrixXY-1: j+1;
for (int tempI = i+2; tempI<=lowerI;tempI++) {
checkSumForSurraundings +=battleFieldMatrix[tempI][leftJ] + battleFieldMatrix[tempI][rightJ];
}
checkSumForSurraundings+=battleFieldMatrix[lowerI][j];
if (checkSumForSurraundings == 0) {
for (int tempI = i;tempI<i+size;tempI++) {
battleFieldMatrix[tempI][j] = size*10+shipNumber;
}
endSuccessfully = 't';
break;
}
}
case 'u':
checkSumForShip = 0;
for (int r = i-1; (r>=i-(size-1)) && (r >0) ;r--) {
if (battleFieldMatrix[i][r] == 0) checkSumForShip++;
}
if (checkSumForShip == size-1) {
checkSumForSurraundings = 0;
upperI = (i-size) < 0 ? 0 : i-size;
leftJ = j == 0 ? 0: j-1;
rightJ = j == battelFieldMatrixXY-1 ? battelFieldMatrixXY-1: j+1;
for (int tempI = i-2; tempI>=upperI;tempI--) {
checkSumForSurraundings +=battleFieldMatrix[tempI][leftJ] + battleFieldMatrix[tempI][rightJ];
}
checkSumForSurraundings+=battleFieldMatrix[upperI][j];
if (checkSumForSurraundings == 0) {
for (int tempI = i;tempI>i-size;tempI--) {
battleFieldMatrix[tempI][j] = size*10+shipNumber;
}
endSuccessfully = 't';
break;
}
}
}
if (endSuccessfully == 't') break;
default:
placeShips(size, shipNumber);
}
}
int randomCell() {
unsigned int i;
unsigned int j;
int upperI, lowerI, leftJ, rightJ;
unsigned int random;
unsigned int checkAroundSum;
while (1) {
random = rand() % (battelFieldMatrixXY*battelFieldMatrixXY);
i = (random - random % 10)/10;
j = random % 10;
checkAroundSum = 0;
if (battleFieldMatrix[i][j] == 0) {
upperI = i == 0 ? 0 : i-1;
lowerI = i == battelFieldMatrixXY-1 ? battelFieldMatrixXY-1 : i+1;
leftJ = j == 0 ? 0 : j-1;
rightJ = j == battelFieldMatrixXY-1 ? battelFieldMatrixXY-1 : j+1;
checkAroundSum = battleFieldMatrix[upperI][j] +
battleFieldMatrix[lowerI][j] +
battleFieldMatrix[i][leftJ] +
battleFieldMatrix[i][rightJ] +
battleFieldMatrix[upperI][leftJ] +
battleFieldMatrix[upperI][rightJ] +
battleFieldMatrix[lowerI][leftJ] +
battleFieldMatrix[lowerI][rightJ];
if (checkAroundSum == 0) break;
}
}
return i*10+j;
}
// ========================== Place_Ships.c
#ifndef Place_Ships_h
#define Place_Ships_h
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include "Global_Data.h"
void placeShips(int size, int shipNumber);
#endif /* Place_Ships_h */
/*
* Try to not use global variables, it is better to pass them around inside functions.
* You may have trouble with them later on if another file(s) "change" their value by accident.
* Other than that, everything here looks cool.
*/
#include "Print_Matrix.h"
void printMatrix(int mode) {
printf(" 0 1 2 3 4 5 6 7 8 9 \n");
printf(" -------------------------------\n");
for (int i = 0; i<battelFieldMatrixXY;i++) {
//the start of each line with art and number
printf("%d | ",i);
for (int j = 0; j<battelFieldMatrixXY;j++) {
switch (battleFieldMatrix[i][j]) {
case 0:
printf(" . ");
break;
case 77:
printf(" × ");
break;
default:
if (mode == 0) printf(" © ");
else printf(" . ");
break;
}
}
printf("|\n");
}
printf(" -------------------------------\n");
}
#ifndef Print_Matrix_h
#define Print_Matrix_h
#include <stdio.h>
#include "Global_Data.h"
// try to use enums next time :)
void printMatrix(int mode);
#endif /* Print_Matrix_h */
// ========================== Start_Game.c
#include <stdio.h>
#include "Print_Matrix.h"
#include "Place_Ships.h"
int main(void)
{
/* You have a warning there, saying:
* Implicit conversion loses integer precision: 'time_t' (aka 'long') to 'unsigned int'
* That happens because 'time_t' is not a 'unsigned int', so it is up to the programmer to do that.
* To fix that, we would write something like srand((unsigned int) time(NULL));
* Have in mind that 'time_t' (aka 'long') is 8 bytes long while 'unsigned int' is still 4 bytes long,
* in this way, when you cast it to (unsigned int), the value is truncated
*/
srand(time(NULL));
placeShips(4,1);
placeShips(3,1);
placeShips(3,2);
placeShips(2,1);
placeShips(2,2);
placeShips(2,3);
placeShips(1,1);
placeShips(1,2);
placeShips(1,3);
placeShips(1,4);
printMatrix(1);
startTheGame();
}
// You don't need to write comments like "Print the Matrix" in a function called 'printMatrix'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment