HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

An unbeatable TicTacToe game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
tictactoegameunbeatable

Problem

I wrote an answer to another question about the same subject and a new implementation, but instead of just posting my code there I think it's a good chance to see what can be improved on my own code.

```
#include
#include
#include
#include
#include
#include
#include

/ Error handling made easy. /
[[noreturn]] void fail(char const * message) {
std::cerr ;

Board make_empty_board(void) {
Board b;
std::fill(std::begin(b), std::end(b), Mark::None);
return b;
}

std::ostream & operator(o, "|"));
o possible_moves(State const & state) {
std::vector moves;
unsigned index = 0;
for (auto const place : state.board) {
if (place == Mark::None) {
moves.emplace_back(index);
}
++index;
}
return moves;
}

State make_move(State state, Move move) {
if (move >= state.board.size() || state.board[move] != Mark::None) {
fail("Invalid move");
}
state.board[move] = state.computersTurn ? Mark::Computer : Mark::Human;
state.computersTurn = !state.computersTurn;
return state;
}

/* Possible winners:
* - Human: The human won (or is about to win).
* - Computer: The computer won (or is about to win).
* - None: Its a tie or it's not yet sure who's about to win.
*/
enum class Winner {
Human,
Computer,
None
};

Winner determine_winner(State const & state) {
Board const & b = state.board;
Mark const lines[][3] = {
/ Horizontal /
{ b[0], b[1], b[2] },
{ b[3], b[4], b[5] },
{ b[6], b[7], b[8] },
/ Vertical /
{ b[0], b[3], b[6] },
{ b[1], b[4], b[7] },
{ b[2], b[5], b[8] },
/ Cross /
{ b[0], b[4], b[8] },
{ b[6], b[4], b[2] }
};
for (auto line : lines) {
if (std::count(line, line + 3, Mark::Human) == 3) {
return Winner::Human;
}
if (std::count(line, line + 3, Mark::Computer) == 3) {
return Winner::Computer;
}
}
return Winner::None;
}

/* Partition the given range into 3 groups, according to the given selector.
*
* Let r be the result of pa

Solution

Prefer using exceptions to exit

/* Error handling made easy. */
[[noreturn]] void fail(char const * message) {
  std::cerr << message << std::endl;
  std::exit(EXIT_FAILURE);
}


I would always throw an exception in preference to exiting the code. In the best case this causes the exact same behavior and causes the program to exit. BUT it also unwinds the stack forcing a clean termination and resource clean up. Also if you code is part of another program (ie this is just one of many games that can be played) you want your game to exit but you don't want the control program to go down. Using an exception allows this as the control program can catch the exception generate an appropriate user message log the error and allow the user to choose an alternative program.

###Switch should have a default:

std::ostream & operator<< (std::ostream & s, Mark m) {
  switch (m) {
    case Mark::None:     return (s << "_");
    case Mark::Computer: return (s << "C");
    case Mark::Human:    return (s << "H");
  }
  // The problem with this fail() is that it is already too late.
  // A switch that has no matching case statement results in 
  // undefined behavior. You should always put a default clause in
  // the switch just to catch this situation (some maintainer may
  // add other option to Mark).
  fail("Not reached");
}


The code should look like this:

std::ostream & operator<< (std::ostream & s, Mark m) {
  switch (m) {
    case Mark::None:     return (s << "_");
    case Mark::Computer: return (s << "C");
    case Mark::Human:    return (s << "H");
    default:             break;  // Just add this line.
  }
  fail("Not reached");
}


###Prefer not to use global variables.

/* A board is just 3 rows of each 3 marks. */
using Board = std::array;


Prefer to use local variables and pass the board as parameter to your functions. A tiny bit of extra work (but by using encapsulation this work is removed see next comment).

Encapsulation

This is indicative of not encapsulating your code into a class.

/* A board is just 3 rows of each 3 marks. */
using Board = std::array;


But having this just means your code is tied to this one board. You can make your code much neater and thus much more re-usable by encapsulating this information into a class with methods to manipulate the state of the board.

Also by using encapsulation you will prevent accidently modification of the board by other parts of the code becuase you can protect the state of the object by making it private.

Code Snippets

/* Error handling made easy. */
[[noreturn]] void fail(char const * message) {
  std::cerr << message << std::endl;
  std::exit(EXIT_FAILURE);
}
std::ostream & operator<< (std::ostream & s, Mark m) {
  switch (m) {
    case Mark::None:     return (s << "_");
    case Mark::Computer: return (s << "C");
    case Mark::Human:    return (s << "H");
  }
  // The problem with this fail() is that it is already too late.
  // A switch that has no matching case statement results in 
  // undefined behavior. You should always put a default clause in
  // the switch just to catch this situation (some maintainer may
  // add other option to Mark).
  fail("Not reached");
}
std::ostream & operator<< (std::ostream & s, Mark m) {
  switch (m) {
    case Mark::None:     return (s << "_");
    case Mark::Computer: return (s << "C");
    case Mark::Human:    return (s << "H");
    default:             break;  // Just add this line.
  }
  fail("Not reached");
}
/* A board is just 3 rows of each 3 marks. */
using Board = std::array<Mark, 3 * 3>;
/* A board is just 3 rows of each 3 marks. */
using Board = std::array<Mark, 3 * 3>;

Context

StackExchange Code Review Q#145085, answer score: 4

Revisions (0)

No revisions yet.