Hcfyk

First of all: nice work! It's easy to read and understand.

Program organization

It's very good that you split the task to small functions.
Reading the body of main reveals nicely the overall flow.

Ideas for further improvement:

• place does two things: it reads input from user and updates the state of the board. It would be good to separate these logically distinct steps to different functions. A new function, say read_next_move could return boolean just like place, and take parameters board and pointers to x and y, whose values will be used when updating the board state.




• The name check is too generic. How about is_game_over or has_player_won.




• Printing the winner would be good to move to a dedicated function, as you did with other steps.

Use more constants

The special symbols -, x and o would be good to define as constants.
So that instead of if (board[pos] == 'x' || board[pos] == 'o'),
you could write more descriptively if (board[pos] == PLAYER_X || board[pos] == PLAYER_O), and if ever needed change the values easily.
By the way, in this particular example I would replace the condition with if (board[pos] != AVAILABLE).

There's two things I'd change to simplify your code:

• Use a two dimensional array. Sure you can do the conversion from a 2d position to 1d easily, but the compiler can do and board[1][1] is rather more obviously the middle of the board than board[4].


• Instead of hardcoding your logic for what positions you have to check, think about a programmatic approach using a loop. This not only avoids duplicating lots of code, but also makes it trivial to extend your tic-tac-toe game to a larger board.

I think youre code is quite nice. Easy to read and the tasks are nicely splitted into functions. So theres not much to add.

Theres just one thing bothering me.

Don't omit curly braces. This:

// Check rows

if (b[0] == p && b[1] == p && b[2] == p)

    return true;

if (b[3] == p && b[4] == p && b[5] == p)

    return true;

if (b[6] == p && b[7] == p && b[8] == p)

    return true;

Should be this:

// Check rows

if (b[0] == p && b[1] == p && b[2] == p) {

    return true;

}

if (b[3] == p && b[4] == p && b[5] == p) {

    return true;

}

if (b[6] == p && b[7] == p && b[8] == p) {

    return true;

}

You wonder why? It can lead to bugs. One example is apple's SSL bug

For more discussion see: https://stackoverflow.com/questions/359732/why-is-it-considered-a-bad-practice-to-omit-curly-braces

The following remarks may be nitpicks. Since your code is already quite good, it's all I have to say. :)

#include <stdio.h>

#include <stdlib.h>

#include <stdbool.h>

Since all the above headers are from the standard C library, they should be in alphabetical order.

When you include other headers like <sys/type.h>, the order is sometimes important. But not in your simple program.

#define SIZE 3

Having this constant means that anyone else may later change the 3 into a 5 and can expect that the program still works reasonably well. If your code doesn't provide this guarantee, you should add a small comment explaining that this constant should not be modified.

// Draws the board along with rows/cols, numbered.

void draw_board(char board)

Function signatures in C should not contain arrays since these behave surprisingly in several cases (like multi-dimensional arrays). Also, it sounds strange to say "to draw the board, given a character array". The function signature would sound a lot better like this:

void draw_board(tic_tac_toe_board *board)

This changes the wording to "draw the board, given a board", which is a bit redundant, but it focuses on the problem domain instead of the technical level, which makes the code easier to understand, especially if you want to explain programming to laymen.

To make the above function signature valid, you need to declare the tic_tac_toe_board as a type:

typedef struct {

    char cell[SIZE * SIZE];

} tic_tac_toe_board;

With this type definition, your code may look like this:

void init_board(tic_tac_toe_board *board)

{

    for (int i = 0; i <  SIZE * SIZE; i++)

        board->cell[i] = '-';

}

It's a bit more effort to write board->cell[i] instead of the simple board[i] from before, but it's worth it since you can now talk about a tic-tac-toe board, without having to mention that it is implemented as a character array.

Having this abstraction also means that you can easily extend the board by recording the history of moves, just in case you want to implement an undo feature later:

typedef struct {

    char cell[SIZE * SIZE];

    int moves_count;

    int moves[SIZE * SIZE];

} tic_tac_toe_board;

Next topic, the communication with the user:

    printf("%c, pick your position (xy, rc): ", player);

That message is a mistake: xy is not the same as rc since r corresponds to y, not x. It should be either (xy, cr) or (yx, rc).

    scanf("%s", posinput);

Instead of reading a string here, there's a completely different idea. Most keyboards have a numeric block, which by coincidence consists of 3×3 keys. When you map the keyboard layout as follows, the human players just need a single key instead of two to enter a coordinate, plus the position on the keyboard matches exactly the position on the board:

7 8 9

4 5 6

1 2 3

It only works for 3×3 though. But if you accept that limitation:

int pos;

if (scanf("%d", &pos) == 1) {

    int row = (SIZE - 1) - (pos - 1) / SIZE;

    int col = (pos - 1) % SIZE;

    int board_pos = col + row * SIZE;

}

Going further in your code:

if (board[pos] == 'x' || board[pos] == 'o')

    return false;

I would rather say board[pos] != '-', for 3 reasons: it is shorter, it is faster, and when you add a third player someday the code is still correct.

// Returns true if there are three of the same chars in a row.

// b = board, p = player. Shortened for readability.

A very nice comment. Short, to the point, and informative.

The rest of the code looks good already. There's just one missing thing. Anecdotal evidence suggests that in the game of tic-tac-toe, players familiar with each other will tie 100% of the time due to the limited number of outcomes. I suggest you implement the tie rule. It's very simple: if SIZE * SIZE moves have been played and there is still no winner, it's a tie.

The code looks clean and readable.

You could rename check to checkForWin to tell what it checks.

I would also suggest to improve that procedure by declaring the lines as a constant array instad of writing a similar test 8 times:

const int lines[8][3] = {

    { 0, 1, 2 }, // rows

    { 3, 4, 5 },

    { 6, 7, 8 },

    { 0, 3, 6 }, // columns

    { 1, 4, 7 },

    { 2, 5, 8 },

    { 0, 4, 8 }, // diagonals

    { 2, 4, 6 }

};



bool checkForWin(char b, char p) 

{

    for (int i = 0 ; i < 8 ; i++ ){

        if (b[lines[i][0]] == p && b[lines[i][1]] == p && b[lines[i][2]] == p){

            return true;

        }

    }

    // No winnig line found

    return false;

}

Starting from the top, defining the SIZE macro is good - this potentially allows you to change the size of the board by modifying one number. It is especially good because it allows you to later come back and change it to the name of a global variable instead of a constant number, which then allows you to define the size of the board runtime.

You then throw away some of that utility in draw_board(), by printing the column numbers as a constant ("# 1 2 3n"), rather than by using the nice macro you defined just a few lines prior. You should print the column numbers with a loop, just like you print the board itself. Consider also writing ones for the symbols (i.e. EMPTY, PLAYER1 and PLAYER2) so you can change them if desired.

init_board() is great, but you could use memset(board, '-', SIZE * SIZE) instead of the loop.

In place(), your scanf() format should not be "%s". You're expecting two numbers (or digits), so scan for those: "%d%d" or "%c%c" will do the job and allow for improvements later down the line. In addition, you can check for whether there already is a symbol at the coordinate in a single comparison with board[pos] != '-'.

Also, you should return the index on success and a faulty value like -1 on failure, so that your check() can take that as input and only check the rows, columns and diagonals that the token is a part of. Something like this:

// In the main loop:

int index = place();

if (index >= 0) 

{

    if (check(board, index, player))

        break; // Placement succeeded and resulted in a win, break loop.

    /* … */

}

Next, in check(), you check a 3x3 board, completely ignoring that nice SIZE macro you defined. You should instead use a loop to check because then you can freely change the SIZE of the board later down the line. Combined with the index for input, you'd have something like...

bool check(char board, int index, char player)

{

    int row = index / SIZE;

    int col = index % SIZE;



    // We'll unconditionally check the diagonals too.

    int rowCount = 0;

    int colCount = 0;

    int diagLCount = 0; // Top-left diagonal

    int diagRCount = 0; // Top-right diagonal



    for (int i = 0; i < SIZE; i++) {

        // Comparisons (< > >= <= != ==) evaluate to a 1 (true) or a 0 (false),

        // so we can just sum them into the counters.

        rowCount += board[row * SIZE + i] == player;

        colCount += board[i * SIZE + col] == player;

        diagLCount += board[i * SIZE + i] == player;

        diagRCount += board[i * SIZE + SIZE - i] == player;

    }



    // Just unconditionally return whether any of the counters reached the desired value.

    return rowCount == SIZE || colCount == SIZE || diagLCount == SIZE || diagRCount == SIZE;

}

Finally, you may want to define a function-like macro or a function for indexing the board to reduce the chance of having a typo cause a bug and to make it somewhat more legible. It can also reduce the amount of work you need to put in if you choose to change the indexing later on.

#define AT(X, Y)   [(Y) * SIZE + (X)]

int at(int x, int y) { return y * SIZE + x; }

// which allow you to write something like...

if ( board AT(x, y) == '-' )

   board[at(x, y)] = 'x';

Overall, this is well-written code. One thing I would change besides what has already been suggested by others is your call to system("clear") which creates an OS-specific dependency.

You can instead use #ifdef to check the platform and then call the appropriate command to clear the screen. Additionally, this can be wrapped in a function to allow for code that is easy to reuse.

Example:

void clear() {

    #ifdef _WIN32

        system("cls");

    #else

        system("clear");

    #endif

}