Console battleship game made in C

Question

I started out to learn more about C to learn more about low level programming and hardware and I can improve.

It generates a random number between 1 and 4 then stores it into two variables.

It takes in to x,y board values ranging to 1,4 in the board then if the two inputs equal the random values it gives a point and resets but if not it marks the board and takes one try away. If the amount of tries reaches 5 then it resets the board

It's basic and meant to only have one target for each round and I did not do any error checking for ascii inputs.

/*---------#include---------*/

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <unistd.h>

/*---------value definitions---------*/
#define BOARD_SIZE 5
#define LOW_BOUNDS 0
#define HIGH_BOUNDS 4
#define TRIES 5

//functions
void DRAW_BOARD();
int GEN_RND_ZERO();
void CLEAR_BOARD();

int I1,I2; //Iterator values

char LAYOUT[BOARD_SIZE][BOARD_SIZE] =
{
    {'X','1','2','3','4'},
    {'1','o','o','o','o'},
    {'2','o','o','o','o'},
    {'3','o','o','o','o'},
    {'4','o','o','o','o'},
};



/*-----MAIN-----*/

int main()
{
    srand(time(NULL)); //rnd val seed

    unsigned short INPUT1,INPUT2;

    int RND1,RND2;

    unsigned short PNT = 0,Counter;

    printf("-Battleship Game-\n");
    sleep(2);

set1:
    RND1 = GEN_RND_ZERO();
    RND2 = GEN_RND_ZERO();
set2:
    printf("\n%d %d\n\n",RND1,RND2);

    DRAW_BOARD();

    printf("\nEnter an X value: "); scanf("%hu",&INPUT1);
    printf("\nEnter a Y value: "); scanf("%hu",&INPUT2);

    //Input error check
        if(INPUT1 <= 0 || INPUT1 > 4 || INPUT2 <= 0 || INPUT2 > 4)
        {
            printf("\n\nInput failure\n\n");
            sleep(3);
            system("cls");
            goto set2;
        }
    if(INPUT1==RND1&&INPUT2==RND2)
    {
        PNT++;
        printf("\n\nPoint increase [%d]\n",PNT);
        CLEAR_BOARD();

        sleep(3); system("cls"); goto set1;
    }
    else
    {
        Counter++;
        printf("\n\nIncorrect\n\n");
        LAYOUT[INPUT1][INPUT2] = 'x';
        if(Counter == TRIES)
        {
            printf("Out of tries");
            Counter = 0;
            sleep(3); system("cls"); goto set1;
        }

        sleep(3); system("cls"); goto set2;
    }

}



/*---------function bodys---------*/

//Draws the player board
void DRAW_BOARD()
{
    for(I1 = 0; I1<BOARD_SIZE; I1++)
    {
        for(I2 = 0; I2<BOARD_SIZE; I2++)
        {
            printf("%c ",LAYOUT[I1][I2]);
        }
        printf("\n");
    }
}

//Replace all marked characters back to an o
void CLEAR_BOARD()
{
    for(I1 = 1; I1<BOARD_SIZE; I1++)
    {
        for(I2 = 1; I2<BOARD_SIZE; I2++)
        {
            LAYOUT[I1][I2] = 'o';
        }
        printf("\n");
    }
}

//Checks if the random value generated is zero if it is then it will increment to 1
int GEN_RND_ZERO()
{
    int temp = rand()%4;

    //Pure Laziness
    if(temp == 0) {
        temp++;
    }
    return temp;
}

Answer 1

Naming conventions

There doesn't seem to be one definite naming convention for C, but there's still something to be said here.

• I haven't seen functions in SCREAMING_SNAKE_CASE, they are usually written in something like snake_case or camelCase
• Your variable naming is inconsistent, as seen by the examples of temp, PNT and Counter. They are all integers local to a functions, so they should be named in a similar style

GEN_RND_ZERO

The comment above the function is misleading, as this function doesn't check anything, it generates something.
If I understood correctly, this is supposed to generate a number between 1 and 4, used for coordinates. The method used however is wrong: rand()%4 will only generate the numbers 0, 1, 2 and 3, and if you increment only if the result is 0, then the result will be 1, 1, 2, 3. This not only leads to the last row/column always being empty, it also leads to the first row/column being more likely to contain a target. The correct way is to use rand() % 4 + 1, creating the desired values 1, 2, 3, 4.

The iterators

Making the for-loop iterators global is a bad idea. They clutter up your namespace without being used somewhere outside of the loops. It may also be confusing to readers. Keep the scope of variable as small as needed! The two "correct" ways to declare and initialize iterators are these:

int i;
for (i = 0; i < 100; i++) { //...

for (int i = 0; i < 100; i++) { //...

The latter one is to be preferred, but it's only possible if the compiler supports the C99 standard. Yours seems to support it, as you are able to make single line comments with // (unless it's a pre-C99 compiler with an extension, which is unlikely these days. You seem to be using a POSIX-compliant environment, so your compiler is most certainly a recent gcc. No problems here.).

Long lines

• sleep(3); system("cls"); goto set2; should be three lines instead of one
• printf("\nEnter an X value: "); scanf("%hu",&INPUT1); should be two lines

Long lines like this I find to be just as readable as multiple lines at best. Why put this on one line, when you can use multiple lines and maybe an empty line after instead?

//...
sleep(3); 
system("cls"); 
goto set2;

// ...

printf("\nEnter an X value: "); 
scanf("%hu",&INPUT1);

printf("\nEnter an Y value: "); 
scanf("%hu",&INPUT2);

goto

[softly]: Don't.

Jokes aside, if you must use goto, use expressive labels. In this case, I'd suggest new_game for set1 and try for set2. Then again, this can (and probably should) be rewritten to use loops instead of gotos (How, I'll leave as an exercise for the reader :)). gotos do have some "good" use though, as they are a nice way to do error handling.

DRAW_BOARD

This function should have another printf("\n"); outside of the outer loop. You probably don't want to print something next to the board and having to print the newline outside of DRAW_BOARD makes it hard to keep track of where to print newlines.

Idea

Why not show the total score reached on game over?

Answer 2

Portability

The following functions are not portable: sleep() and system("cls"). I had to add this to get it to compile with MSVC

#ifdef _WIN32
#include <Windows.h>
#define sleep(x) Sleep(x * 1000)
#endif

system("cls"); will clear screen on Windows but fail on all other operating systems.

---

Uninitialized variables

Consider the following line:

unsigned short PNT = 0, Counter;

Why did you give an initial value for PT but not for Counter?

---

Input validation
See what happens if you enter a non-numeric value. The program goes into a crazy loop. Consider creating a function to validate your input instead of using `scanf`:

int readNumber(int minVal, int maxVal)
{
retry_input:
    char buf[BUFLEN];
    if (fgets(buf, BUFLEN, stdin) == 0)
        return -1;
    int value = strtol(buf, 0, 10);
    if (value < minVal || value > maxVal)
    {
        printf("Please enter a number between %d and %d: ", minVal, maxVal);
        goto retry_input;
    }
    return value;
}

And changing main to the following:

printf("\nEnter an X value: ");
INPUT1 = readNumber(1, 4);
if (INPUT1 == -1)
    return EXIT_FAILURE;

printf("\nEnter a Y value: "); 
INPUT2 = readNumber(1, 4);
if (INPUT2 == -1)
    return EXIT_FAILURE;

if (INPUT1 == RND1 && INPUT2 == RND2)
{
    ...