I have already posted this program and got some good recommendations on how it can be improved but I would like to further know what can be improved for example:
- Should dynamic memory allocation be used and if yes, where should I allocate it ?
- How can I replace system("cls") in the program ?
- Should I get rid the global variable ?
- I got recommend to enable all warnings I couldn't figure out how to do that
#include<stdlib.h>
#include<stdio.h>
#include<stdbool.h>
#define NUM_MIN 1
#define NUM_MAX 9
#define LINE_SIZE 80
void print_board();
int get_input(int player);
bool check_input(int user_input);
void change_board(int user_input, int player);
bool check_win(char arr_values[3]);
bool player_has_won();
char g_cell_values[9] = {'1','2','3','4','5','6','7','8','9'};
int main(){
int players_turn[9] = {1,2,1,2,1,2,1,2,1};
int cell_num = 0;
for(int i = 0; i < 9; i++)
{
system("cls");
print_board();
cell_num = get_input(players_turn[i]);
change_board(cell_num,players_turn[i]);
if(i == 8)
{
system("cls");
print_board();
}
if(player_has_won())
{
system("cls");
print_board();
printf("player %d wins \n",players_turn[i]);
return 0;
}
}
printf("its a draw\n");
return 1;
}
void print_board(){
printf("\n");
int j = 0;
for (int i = 0; i < 2; i++)
{
if(i == 1)
{
j = 3;
}
printf(" %c | %c | %c\n",g_cell_values[j],g_cell_values[j+1],g_cell_values[j+2]);
printf("_____|_____|_____\n");
printf(" | |\n");
}
j = 6;
printf(" %c | %c | %c\n",g_cell_values[j],g_cell_values[j+1],g_cell_values[j+2]);
}
int get_input(int player){
int num = 0;
char buf[2*LINE_SIZE];
do {
printf("player %d enter a number option ", player);
if(fgets(buf, sizeof buf, stdin) == NULL) {
printf("error occurred\n");
exit(EXIT_FAILURE);
}
} while (sscanf(buf, "%d", &num) != 1 || num < NUM_MIN || num > NUM_MAX);
return num;
}
bool check_input(int user_input){
if(g_cell_values[user_input-1] == 'O' || g_cell_values[user_input-1] == 'X')
{
return false;
}
else
{
return true;
}
}
void change_board(int user_input, int player){
int new_input = 0;
if(check_input(user_input))
{
if(player == 1)
{
g_cell_values[user_input-1] = 'X';
}
else
{
g_cell_values[user_input-1] = 'O';
}
return;
}
else
{
new_input = get_input(player);
change_board(new_input,player);
}
}
bool check_win(char arr_values[3]){
int count = 0;
int count2 = 0;
for(int i =0; i<3; i++)
{
if(arr_values[i] == 'X')
{
count++;
}
else if(arr_values[i] == 'O')
{
count2++;
}
}
if(count == 3 || count2 == 3)
{
return true;
}
else
{
return false;
}
}
bool player_has_won(){
char cell_values[3];
char cell_values2[3];
for(int i = 0; i < 3; i++)
{
for(int j = 0; j < 3; j++)
{
cell_values[j] = g_cell_values[i * 3 + j];//rows
cell_values2[j] =g_cell_values[j * 3 + i];//columns
}
if(check_win(cell_values) || check_win(cell_values2))
{
return true;
}
}
int j =0;
for(int i =0; i<3; i++)
{
cell_values[i] = g_cell_values[i * 3 + j];//left to right across
j++;
}
int k = 2;
for(int i =0; i < 3; i++)
{
cell_values2[i] = g_cell_values[i*3 + k];//right to left across
k--;
}
if(check_win(cell_values) || check_win(cell_values2))
{
return true;
}
else
{
return false;
}
}
2 Answers 2
1. Dynamic memory allocation
There is no need to dynamically allocate memory for the application.
2. Clearing the screen
There is no portable way to clear the screen. The following works on most operating systems:
system("cls||clear");
I would recommend creating a separate function to handle this so that if you need to change it, you would only need to do so in one location:
void clear_screen() {
system("clear||cls");
}
3. Global variables
I think that using global variables for this specific solution is fine. You could pass cell_values
to the functions directly if you did not want to have one:
int get_input(char cell_values[9], int player);
int main() {
char cell_values[9] = { '1','2','3','4','5','6','7','8','9' };
int players_turn[9] = { 1,2,1,2,1,2,1,2,1 };
int cell_num = get_input(cell_values, players_turn[i]);
}
4. Enabling warnings
This will really depend on what compiler you are using.
More notes
You should call check_input
from get_input
instead of recursively calling change_board
:
int get_input(int player) {
int num = 0;
char buf[2 * LINE_SIZE];
do {
printf("player %d enter a number option ", player);
if (fgets(buf, sizeof buf, stdin) == NULL) {
printf("error occurred\n");
exit(EXIT_FAILURE);
}
} while (sscanf(buf, "%d", &num) != 1 || check_input(num) == false);
return num;
}
bool check_input(int user_input) {
if (user_input < NUM_MIN || user_input > NUM_MAX)
return false;
char cell_value = g_cell_values[user_input - 1];
return cell_value == 'O' || cell_value == 'X' ? false : true;
}
void change_board(int user_input, int player) {
g_cell_values[user_input - 1] = player == 1 ? 'X' : 'O';
}
You do not need an if
statement if you are simply going to return a bool
. In the last part of check_win
you could have simply used:
return count == 3 || count2 == 3;
The same applies to player_has_won
:
return check_win(cell_values) || check_win(cell_values2);
-
\$\begingroup\$ I am using mingw gcc compiler if it helps, but do you think its a good idea to enable all warnings ? \$\endgroup\$BobTheCoder– BobTheCoder2021年10月29日 18:16:26 +00:00Commented Oct 29, 2021 at 18:16
-
\$\begingroup\$ @BobTheCoder, I really don't know gcc well enough to help. \$\endgroup\$jdt– jdt2021年10月29日 18:49:05 +00:00Commented Oct 29, 2021 at 18:49
Enabling warnings
I mostly see and use -Wall -Wextra
. Is it a good idea? Yes, I think so. More often than not, the warnings turn out to be inaccuracies that cause bugs. Be sure to read and understand them, as in some cases the warning is a false alarm. As an example, I've had gcc complain about gethostname
being undefined while unistd.h
was included and the program still worked fine.
I don't recommend using -Werror
, especially while initially writing the program. gcc will then throw every warning as an error, so you won't be able to ignore some warnings that you know are fine when you want to test something quickly (e.g. unused functions).