Reasonably clean & tidy two player game.
Consider swapping type int for type short or byte, since they took up less space and you don’t need to store big values.
It was a challenge to come up with an AI that plays Tic-Tac-Toe, but it will be good to have one.
https://www3.ntu.edu.sg/home/ehchua/programming/java/JavaGame_TicTacToe_AI.html
I made the changes that @Pharap recommended. Thanks for that. Oh and I found out it is arduboy.pollButtons() not arduboy.pullButtons(). I had to look that up 
. And I would love to see what else you have to teach me.
The last time I did any C was in High School (1999). I have done some other languages QBasic, VB(before .NET), Java, PHP… It has been a few years since I had really coded anything and thought this would be fun.
@CDR_Xavier I want to do an AI so thanks for the Link, I’ll Take a look.
My son and I wrote a version of ‘Noughts and Crosses’ that had both a two-player and ‘opay-against-the-computer’ mode. When playing against the computer, there is no AI it simply follows the rules:
Hello, very nice game! I like the code, very good for a first project.
Improvements you could do:
I can see a lot of long copy-paste conditions like this one:
if(board[0][0]==board[1][1] && board[0][0]==board[2][2] && board[1][1]==board[2][2] && board[0][0]!=0)
For this it would be very good to make a function or a macro. Loops will be better probably.
Also if you analyze it a bit you can see that you don’t have to check board[1][1]==board[2][2] because if the first two conditions are true, this one has to be too (via a property called transitivity).
You could also make the condition evaluate faster if you put board[0][0]!=0 first, because if the square is zero, then the previous two conditions will have been evaluated needlessly. So all in all it could be rewritten as
if(board[0][0]!=0 && board[0][0]==board[1][1] && board[0][0]==board[2][2])
EDIT:
Here is what it might look like:
// check rows
for (unsigned char i = 0; i < 3; ++i)
if(board[0][i]!=0 && board[0][i]==board[1][i] && board[0][i]==board[2][i])
win();
// check columns
for (unsigned char i = 0; i < 3; ++i)
if(board[i][0]!=0 && board[i][0]==board[i][1] && board[i][0]==board[i][2])
win();
// check diagonal 1
if(board[0][0]!=0 && board[0][0]==board[1][1] && board[0][0]==board[2][2])
win();
// check diagonal 2
if(board[2][0]!=0 && board[2][0]==board[1][1] && board[2][0]==board[0][2])
win();
Clearly I misspelt it on purpose to trick you into looking it up.
I certainly didn’t press the wrong key on the keyboard by accident. :P
To clarifty, this is C++, not C.
Although Arduino boards don’t have a C++ stdlib implementation, they use avr-libc which is an implementation of the C standard library, which is why a lot of Arduino code ends up looking like C.
Swapping out int for a smaller type is a good suggestion, but short is the same size as int on Arduboy, and byte is a bad choice because it’s AVR-specific so using byte will leave you with a bad habbit.
I’d say a loop within a function would be better.
Definitely not a macro. Macros are evil should generally be avoided.
Firstly, as I mentioned above, short and int are the same size on Arduboy.
The basic types in C++ are defined based on their minimum size, not on an absolute size.
int is at least 16 bits, but on 32-bit processors it’s usually 32 bits and on 64-bit processors it’s usually 64 bits.
short is at least 16 bits and usually is 16 bits.
long is at least 32 bits and usually is 32 bits.
long long is at least 64 bits and usually is 64 bits.
To be safe, you’re best off using the fixed-width integer types, which come from the cstdint header (or on Arduino, stdint.h).
Those are:
int8_t and uint8_t are guaranteed to be 8 bits wide.
int16_t and uint16_t are guaranteed to be 16 bits wide.
int32_t and uint32_t are guaranteed to be 32 bits wide.
int64_t and uint64_t are guaranteed to be 64 bits wide.
(When using normal C++ you’d stick a std:: in front of all of those, e.g. std::uint8_t)
So in your case you’re best off swapping to int8_t or uint8_t.
int8_t can store values from -128 to +127.
uint8_t can store values from 0 to 255.
My second bit of advice is to wrap your strings in the Arduino F macro.
Presumably you already have some idea about the difference between RAM and PROGMEM (if you don’t, don’t hesitate to ask).
A bare string (technically called “a string literal”) on Arduino is stored in progmem and then copied into RAM before it’s used.
Wrapping a bare string in the F macro stores it in progmem and returns a special type that print functions (like arduboy.print) recognise as being in progmem, so they can print the strings from progmem.
My third bit of advice is to split your code up into functions more.
For the most part you do a good job, but functions aren’t just for avoiding repetition, they’re also for making the code clearer and more meaningful.
Firstly, you could this (which I notice is repeated twice):
arduboy.fillRect(56, 8, 1, 48, WHITE);
arduboy.fillRect(72, 8, 1, 48, WHITE);
arduboy.fillRect(40, 24, 48, 1, WHITE);
arduboy.fillRect(40, 40, 48, 1, WHITE);
And put it into a function called drawGrid.
Simply doing that automatically makes it clear that these aren’t just rectangles that you’re drawing, but that this is where you’re drawing the grid.
And secondly, even though it only appears once, changing this:
for( int ax=0; ax < 3; ax = ax + 1){
for(int ay=0; ay < 3; ay = ay + 1){
board[ax][ay]=0;
}
}
Into a clearBoard function would make the code more readable.
Then you’re presenting the reader with a clear statement that “I am clearing the board here” rather than making them unpick the loop to understand what you’re doing.
(While I’m at this loop, I’d like to mention that technically int shouldn’t be used to index an array, size_t should be used, but since size_t is 16 bits on Arduboy and the AVR processor is 8 bit, most people iterate through arrays with uint8_t because it produces smaller code and runs slightly faster.)
And a tiny fourth bit of advice, I’d advise extending the ‘zero indexing’ thing to player, so player 1 is stored as ‘0’ and player 2 is stored as ‘1’.
It should make some of your logic run a bit smoother because it allows player == 1 to become player == 0 and the processor has a special way of handling x == 0 and x != 0 that makes comparison with 0 smaller and faster than comparison with other numbers.
I’ve got two final bits of advice, but they’re big ones, so I’ll let you process all of this first and I’ll post the two final bits when you ask for them.
First thank you for spending the time to go over may code. I have only written sloppy/not optimized code on a PC where I had more Ram and Storage then I could ever use. I think it is cool to start to understand and work with in the limitations of the hardware.
What would you recommend for a value to clear the board? I am currently using 0… Should I make it 3?
No problem. I generally enjoy reviewing code.
I once thought 16GB was more than I could ever use,
and then I wrote a program that ran out of RAM :P
(I eventually found a way to make it use less RAM, but it took me several failed attempts to realise the better way of doing it.)
Oh, I see. I didn’t notice you were trying to make the cell values and the player indicators match.
In that case don’t worry about the player numbers for now.
(One of my two remaining bits of advice is about how to better represent the values of the cells in the board.)
Let me know if/when you want me to post the remaining two tips.
I may have gone overboard with Functions I used the recommendation that @drummyfish suggested but used uint8_t instead of unsigned char.
@Pharap Ok I’m ready for your next 2… Does one happen to be about a Main Menu. If not I still plan on adding one. That way I can implement a Computer Player option. Instead of just resetting the game I will jump back to that menu so the user can choose.
You haven’t, in fact you’ve ended up pre-empting some of my suggestions slightly :P
uint8_t is technically the same as unsigned char on the Arduboy, but they have slightly different meanings semantically.
The uint8_t type isn’t present when compiling for processors that can’t handle an 8 bit value (it’s extremely rare, but such processors do exist).
I prefer to use uint8_t because it signals that “this code is expecting to use an 8-bit type and might not work for other types”.
If you use unsigned char, I’d take it to mean “this type expects something at least 8 bits wide, but will work with larger types too”, because the definition of unsigned char as per the C++ standard is ‘at least 8 bits’.
I assumed you were either planning to add one or intentially left it out for simplicity.
I wrote most of this before your recent edits, so if you’ve already made some of these changes then don’t worry.
Don’t be afraid to question some of my suggestions if you aren’t sure why I’ve suggested or think there’s an alternative that will work just as well or even better.
I usually have a reason for doing something a particular way that isn’t always immediately obvious,
but sometimes I just do things out of preference.
My fifth bit of advice is to use ‘scoped enumerations’ (a.k.a. enum classes).
Instead of having an unspoken rule that 0 is ‘none’, 1 is ‘cross’ and 2 is ‘nought’, you can make that rule explicit in your code.
enum class Cell : uint8_t
{
None,
Cross,
Nought,
};
(The : uint8_t bit forces it to be 8 bits in size.)
Then you can define your board as:
Cell board[3][3] =
{
{ Cell::None, Cell::None, Cell::None, },
{ Cell::None, Cell::None, Cell::None, },
{ Cell::None, Cell::None, Cell::None, },
};
Which means instead of doing if(board[selectx][selecty] == 0) you then do if(board[selectx][selecty] == Cell::None) and instead of doing board[selectx][selecty] = player you’d do something like: board[selectx][selecty] = (player == 1) ? Cell::Cross : Cell::Nought.
And then you could take it a step further and combine that with @drummyfish’s code to not only detect a win, but detect who won:
Cell calculateWinner(void)
{
// Check rows
for (uint8_t i = 0; i < 3; ++i)
{
Cell cell = board[0][i];
if((cell != Cell::None) && (cell == board[1][i]) && (cell == board[2][i]))
return cell;
}
// Check columns
for (uint8_t i = 0; i < 3; ++i)
{
Cell cell = board[i][0];
if((cell != Cell::None) && (cell == board[i][1]) && (cell == board[i][2]))
return cell;
}
Cell cell = board[1][1];
if(cell != Cell::None)
{
// Check diagonal 1
if((cell == board[0][0]) && (cell == board[2][2]))
return cell;
// Check diagonal 2
if((cell == board[2][0]) && (cell == board[0][2]))
return cell;
}
return Cell::None;
}
Note that I’ve also cached the values so the code doesn’t have to do keep doing the same array lookup,
and I’ve taken the transitivity a step further by caching the middle cell for evaluating the diagonals.
Using this you can do:
Cell winner = calculateWinner();
// By passing the winner to 'win', you can alter the win message
if(winner != Cell::None)
win(winner);
And my final bit of advice - avoid delay like the plague.
Instead, prefer to structure your code like a state machine.
The simplest way to do that is an enum class and a switch statement, e.g. have:
enum class GameState : uint8_t
{
Reset,
Play,
Win,
Tie,
};
GameState currentState; = GameState::Reset;
void changeState(GameState state)
{
currentState = state;
}
And then in loop you can:
void loop(void)
{
if(!arduboy.nextFrame())
return;
arduboy.pollButtons();
arduboy.clear();
switch(currentState)
{
case GameState::Reset:
updateResetState();
break;
case GameState::Play:
updatePlayState();
break;
case GameState::Win:
updateWinState();
break;
case GameState::Tie:
updateTieState();
break;
}
arduboy.display();
}
Incorporating these suggesntions should mean that your whole code ends up looking roughly like this:
#include <Arduboy2.h>
uint8_t selectx = 1;
uint8_t selecty = 1;
uint8_t player = 1;
uint8_t turn = 1;
enum class GameState : uint8_t
{
Reset,
Play,
Win,
Tie,
};
GameState currentState; = GameState::Reset;
void changeState(GameState state)
{
currentState = state;
}
Cell winner;
void win(Cell winnerCell)
{
winner = winnerCell;
changeState(GameState::Win);
}
void tie(void)
{
changeState(GameState::Tie);
}
void loop(void)
{
if(!arduboy.nextFrame())
return;
arduboy.pollButtons();
arduboy.clear();
switch(currentState)
{
case GameState::Reset:
updateResetState();
break;
case GameState::Play:
updatePlayState();
break;
case GameState::Win:
updateWinState();
break;
case GameState::Tie:
updateTieState();
break;
}
arduboy.display();
}
void clearBoard(void)
{
for(uint8_t x = 0; x < 3; ++x)
for(uint8_t y = 0; y < 3; ++y)
board[x][y] = Cell::None;
}
void drawGrid(void)
{
arduboy.fillRect(56, 8, 1, 48, WHITE);
arduboy.fillRect(72, 8, 1, 48, WHITE);
arduboy.fillRect(40, 24, 48, 1, WHITE);
arduboy.fillRect(40, 40, 48, 1, WHITE);
}
void drawBoard(void)
{
for(uint8_t x = 0; x < 3; ++x)
for(uint8_t y = 0; y < 3; ++y)
{
const char * sprite = nullptr;
if(board[x][y] == Cell::Cross)
sprite = crossSprite;
else if(board[x][y] == Cell::Nought)
sprite = noughtSprite;
arduboy.drawBitmap(40 + (x * 16), 16 + (y * 16), sprite, 16, 16);
}
}
void updateResetState(void)
{
if(arduboy.justPressed(A_BUTTON))
changeState(GameState::Play);
selectx = 1;
selecty = 1;
player = 1;
turn = 1;
winner = Cell::None;
clearBoard();
drawGrid();
arduboy.print(F("Press A to play"));
}
void updateTieState(void)
{
if(arduboy.justPressed(A_BUTTON))
changeState(GameState::Reset);
arduboy.setCursor(48, 57);
arduboy.print(F("Tie!!!"));
}
void updateWinState(void)
{
if(arduboy.justPressed(A_BUTTON))
changeState(GameState::Reset);
arduboy.setCursor(36, 57);
arduboy.print((winner == Cell::Cross) ? F("Crosses") : F("Noughts"));
arduboy.print(F(" Wins!!!"));
}
void updatePlayState(void)
{
if(arduboy.justPressed(LEFT_BUTTON))
if(selectx > 0)
--selectx;
else
selectx = 2;
if(arduboy.justPressed(RIGHT_BUTTON))
if(selectx < 2)
++selectx;
else
selectx = 0;
if(arduboy.justPressed(UP_BUTTON))
if(selecty > 0)
--selecty;
else
selecty = 2;
if(arduboy.justPressed(DOWN_BUTTON))
if(selecty < 2)
++selecty;
else
selecty = 0;
arduboy.print(F("Player "));
arduboy.print(static_cast<int>(player));
if(arduboy.justPressed(A_BUTTON))
if(board[selectx][selecty] == Cell::None)
{
board[selectx][selecty] = ((player == 1) ? Cell::Cross : Cell::Nought);
++turn;
player = ((player == 1) ? 2 : 1);
}
drawBoard();
drawGrid();
Cell winnerCell = calculateWinner();
Cell winner = calculateWinner();
if(winner != Cell::None)
win(winner);
if(turn > 9)
tie();
}
Cell calculateWinner(void)
{
// Check rows
for (unsigned char i = 0; i < 3; ++i)
{
Cell cell = board[0][i];
if((cell != Cell::None) && (cell == board[1][i]) && (cell == board[2][i]))
return cell;
}
// Check columns
for (unsigned char i = 0; i < 3; ++i)
{
Cell cell = board[i][0];
if((cell != Cell::None) && (cell == board[i][1]) && (cell == board[i][2]))
return cell;
}
Cell cell = board[1][1];
if(cell != Cell::None)
{
// Check diagonal 1
if((cell == board[0][0]) && (cell == board[2][2]))
return cell;
// Check diagonal 2
if((cell == board[2][0]) && (cell == board[0][2]))
return cell;
}
return Cell::None;
}
Is this kind of like creating a user defined variable type? Or is this just defining the only values that can be in the cell? or both? or do I just need to learn more C++
Im not sure I follow this syntax. Is this an if statment
if(player==1) use Cell::Cross else use Cell::Nought
Is this static_cast<int> like the F() macro in that is saves space in RAM and is it ok that player is an uint8_t not an int
With states… I would make a new state for menu ie. a GameState::Menu
I like this, I can see how I can use this in other game types… such as equipment screens and such.
It is creating a user-defined type, and it is sort of defining the only values that Cell can hold.
(Although there is a sneaky way to make it hold other values, but you’d have to be trying to do that on purpose.)
Essentially it’s just syntactic sugar for what you were doing before, and it compiles the same as using bare numbers.
However scoped enumeration have some important advantages.
Firstly they add some important meaning to your code - you are no longer dealing with 1s and 2s, you are dealing with actual Noughts and Crosses.
This makes your code clearer and more explicit.
Secondly they’re safer because you cant accidentally assign incorrect values (which isn’t true with unscoped enumerations, or ‘plain enums’).
For example, if you were using numbers you could have done board[x][y] = 11 by mistake (e.g. you pressed 1 twice by accident),
but by using a scoped enumeration that is now a compiler error.
Instead you can only use one of the values accepted by the enumeration type, so you must use a valid value or your code won’t even compile.
(You may want to look at this other comment for some general examples.)
Generally you won’t care what the enumeration values are, but there are rare cases where you might. If you don’t specify any values, the first value is 0 and the rest of the values are given values in ascending order.
In other words this:
enum class Cell : uint8_t
{
None,
Cross,
Nought,
};
Is equivalent to:
enum class Cell : uint8_t
{
None = 0,
Cross = 1,
Nought = 2,
};
You always need to learn more C++.
You can never know too much C++ :P
Basically.
It’s like an if statement, but it’s not a statement - it’s an expression, which is to say “it has a resulting value”.
board[selectx][selecty] = ((player == 1) ? Cell::Cross : Cell::Nought);
is equivalent to:
if(player == 1)
board[selectx][selecty] = Cell::Cross;
else
board[selectx][selecty] = Cell::Nought;
It’s called a ‘ternary expression’ and the combined ?: is called the ‘ternary operator’.
A lot of other languages have this, C# does, I’m pretty sure Java does, PHP probably does.
It’s to force arduboy.print to recognise player as a number and not a character.
I’d need to double check but I’m pretty sure print interprets unsigned char, signed char and char as characters and tries to print them all as characters instead of numbers (and uint8_t and int8_t are just type aliases for unsigned char and signed char on Arduboy - i.e. they’re the same types but with different names).
static_cast is a C++ style cast, as opposed to a C style cast which would have been arduboy.print((int)player).
C-style casts usually look simpler, but they’re actually more complicated and you should avoid them.
C++ has static_cast, const_cast, reinterpret_cast and dynamic_cast, all of which do different things, and some are safer than others.
(static_cast is the safest, dynamic_cast is safe, but has a runtime cost, const_cast and reinterpret_cast are dangerous.)
C style casting makes no distinction. It tries several different combinations of casts until it finds one that works, starting with const_cast and ending with reinterpret_cast (both of which are ‘dangerous’ casts).
For more info about the different kinds of casts, read this.
Yep, that’s the idea.
Pretty much every game has a state machine at its heart.
In larger games you get more sophisticated state machines that do tricks to save memory.
You can also have smaller state machines handling other aspects, like what your character is currently doing.
I recommend reading this article:
http://gameprogrammingpatterns.com/state.html
(Note that although the author uses a plain enum, scoped enums are better. At the time they wrote it, scoped enums were still really new and they didn’t have time to rewrite it.)
“A test to see if you understand it or not.”
int8_t then.
I like the graphics interface (specifically the selection square) and can’t wait to implement a AI for it!
Let me do more research…
Pieceing together some basic understanding of the “MiniMax” annd some junky programming skills produce the following results:
int8_t array[9] = {0,0,0,
0,0,0,
0,0,0}; //the playing field
//for calculating purpose:
//empty = 0
//computer = 1
//player = 2
//when there is no player, computer is encouraged
#define M1 array[0] + array[3] + array[6]
#define M2 array[1] + array[3] + array[7]
#define M3 array[2] + array[4] + array[8]
#define M4 array[0] + array[1] + array[2]
#define M5 array[3] + array[4] + array[5]
#define M6 array[6] + array[7] + array[8]
#define M7 array[0] + array[4] + array[8]
#define M8 array[2] + array[4] + array[6]
int8_t compare[8] = {M1, M2, M3, M4, m5, M6, M7, M8};
void AICompare() {
uint8_t tmpA, tmpB;
for (int8_t A = 0; A < 9; A++) {
if (compare[A] > tmpA) {
tmpA = compare[A];
tmpB = A;
}
}
void setup() {
}
void loop() {
...
...
...
switch(tmpB) {
case 0: if (array[0] == 0) {
array[0] == 1;
break;
} else if (array[3] == 0) {
array[3] == 1;
break;
} else if (array[6] == 0) {
array[6] == 1;
break;
}
case 1: if (array[1] == 0) {
array[1] == 1;
break;
} else if (array[4] == 0) {
array[4] == 1;
break;
} else if (array[7] == 0) {
array[7] == 1;
break;
}
//intentional dropthrough in case some pieces cause unwanted attention
case 2: if (array[2] == 0) {
array[2] == 1;
break;
} else if (array[5] == 0) {
array[5] == 1;
break;
} else if (array[8] == 0) {
array[8] == 1;
break;
}
case 3: if (array[0] == 0) {
array[0] == 1;
break;
} else if (array[1] == 0) {
array[1] == 1;
break;
} else if (array[2] == 0) {
array[2] == 1;
break;
}
case 4: if (array[3] == 0) {
array[3] == 1;
break;
} else if (array[4] == 0) {
array[4] == 1;
break;
} else if (array[5] == 0) {
array[5] == 1;
break;
}
case 5: if (array[6] == 0) {
array[6] == 1;
break;
} else if (array[7] == 0) {
array[7] == 1;
break;
} else if (array[8] == 0) {
array[8] == 1;
break;
}
case 6: if (array[0] == 0) {
array[0] == 1;
break;
} else if (array[4] == 0) {
array[4] == 1;
break;
} else if (array[8] == 0) {
array[8] == 1;
break;
}
case 7: if (array[2] == 0) {
array[2] == 1;
break;
} else if (array[4] == 0) {
array[4] == 1;
break;
} else if (array[6] == 0) {
array[6] == 1;
break;
}
}
ignnore the indenting issues…
I bet this thing, as of how, don’t work significantly better than a randomized placing of pieces.
However, it would be more intelligent than a randomizer…
if position 0 and 3 are held by the computer and 6 is blank then sum would be 2.
If position 0 is held by the player and positions 3 and 6 are empty then the sum would also be 2.
How do you ensure the first scenario is ranked higher as this is a winning move?
Or is this code only for defensive moves?
Good question…
What would this yield?
max(2,2)n
Never mind. I was using < operator.
That is why I say that my bit of code was pretty trash, and might even be worse than a randomizer.
It was the right idea (to count who was holding the pieces on the board) but I was not doing it right…
and yes, that was minimax. simpler? I think that is what the website taught me.(well I didn’t finish reading, so that might indeed NOT be the whole thing)
Well I DID NOT finished the whole thing. Turns out they had the code for minimax on there.
Well maybe we can have mine fixed and let it be a simpler one for some…bigger games, let’s say.
That said, I will take a look at the minimax code and might want to go ahead and pull that bit of code down here so we can use it, since I am the first person to suggest it.
@CDR_Xavier what is it the whole code supposed to do? Are you really trying to implement minimax that looks multiple steps ahead? Or just something simpler?
EDIT: I think minimax won’t be that easy to do and also it’s probably an overkill (at least for such a small board). It could be better to just create an AI that looks at patterns and applies some rules, such as:
There’s a more fundamental problem than this.
The array is defined globally, so M1 to M8 will be evaluated at the point where globals are initialised, which is at the very start of the program.
In other words, it might as well say:
int8_t compare[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
Because that’s exactly what it evaluates to.
Secondly, tmpA and tmpB are local, so if the compiler is as smart as it should be, the entire AICompare function can be reduced to:
void AICompare()
{
}
(Yes, that’s an empty function.)
Thirdly, this probably won’t even compile because in the switch within loop,
array[0] == 1;, array[3] == 1;, array[6] == 1; etc should cause an error along the lines of “an expression cannot be a statement”.
I think you’re getting the min and max functions confused with “the minimax theorem”.
The two things are only tangentially related.
There’s a fairly good example of how to do minimax on noughts and crosses here:
http://how2examples.com/artificial-intelligence/minimax
This sounds pretty much like a definition of most human strategies.
It might even be too good and might result in too many draws, so it might have to be mixed up a bit.
E.g. allow the AI to randomly make a stupid move.
That is a good idea!
And it is not too complicated.
Typing up some code in another word document…
Oops, my bad. (pleasant side effect of writing code dsegments in a word document.)
tmpB should be global. tmpA can be local.
I am calling it that way just because other are. (which is BAD)
Yes, it is very far from the minimax (from Wikipedia)
That would solve one issue, but still leaves lots of other bugs, like tmpA not being initialised.