Problem
I’ve been learning C#, and today I made a simple Tic Tac Toe game. The code is pretty long, and I’m almost 100% positive that this can be done so much better. Please provide some advice, suggestions, and criticism.
byte _clicks = 1;
bool but1clicked, but2clicked, but3clicked, but4clicked, but5clicked, but6clicked, but7clicked, but8clicked, but9clicked = false;
public void ResetGame()
{
but1.Text = null;
but2.Text = null;
but3.Text = null;
but4.Text = null;
but5.Text = null;
but6.Text = null;
but7.Text = null;
but8.Text = null;
but9.Text = null;
but1clicked = false;
but2clicked = false;
but3clicked = false;
but4clicked = false;
but5clicked = false;
but6clicked = false;
but7clicked = false;
but8clicked = false;
but9clicked = false;
}
public void Check()
{
if (but1.Text == "X" && but2.Text == "X" && but3.Text == "X")
{
MessageBox.Show("Game Over! X Wins!");
ResetGame();
}
else if (but4.Text == "X" && but5.Text == "X" && but6.Text == "X")
{
MessageBox.Show("Game Over! X Wins!");
ResetGame();
}
else if (but7.Text == "X" && but8.Text == "X" && but9.Text == "X")
{
MessageBox.Show("Game Over! X Wins!");
ResetGame();
}
else if (but1.Text == "X" && but4.Text == "X" && but7.Text == "X")
{
MessageBox.Show("Game Over! X Wins!");
ResetGame();
}
else if (but2.Text == "X" && but5.Text == "X" && but8.Text == "X")
{
MessageBox.Show("Game Over! X Wins!");
ResetGame();
}
else if (but3.Text == "X" && but6.Text == "X" && but9.Text == "X")
{
MessageBox.Show("Game Over! X Wins!");
ResetGame();
}
else if (but1.Text == "X" && but5.Text == "X" && but9.Text == "X")
{
MessageBox.Show("Game Over! X Wins!");
ResetGame();
}
else if (but3.Text == "X" && but5.Text == "X" && but7.Text == "X")
{
MessageBox.Show("Game Over! X Wins!");
ResetGame();
}
if (but1.Text == "O" && but2.Text == "O" && but3.Text == "O")
{
MessageBox.Show("Game Over! O Wins!");
ResetGame();
}
else if (but4.Text == "O" && but5.Text == "O" && but6.Text == "O")
{
MessageBox.Show("Game Over! O Wins!");
ResetGame();
}
else if (but7.Text == "O" && but8.Text == "O" && but9.Text == "O")
{
MessageBox.Show("Game Over! O Wins!");
ResetGame();
}
else if (but1.Text == "O" && but4.Text == "O" && but7.Text == "O")
{
MessageBox.Show("Game Over! O Wins!");
ResetGame();
}
else if (but2.Text == "O" && but5.Text == "O" && but8.Text == "O")
{
MessageBox.Show("Game Over! O Wins!");
ResetGame();
}
else if (but3.Text == "O" && but6.Text == "O" && but9.Text == "O")
{
MessageBox.Show("Game Over! O Wins!");
ResetGame();
}
else if (but1.Text == "O" && but5.Text == "O" && but9.Text == "O")
{
MessageBox.Show("Game Over! O Wins!");
ResetGame();
}
else if (but3.Text == "O" && but5.Text == "O" && but7.Text == "O")
{
MessageBox.Show("Game Over! O Wins!");
ResetGame();
}
}
public void Tie()
{
if (but1clicked == true &&
but2clicked == true &&
but3clicked == true &&
but4clicked == true &&
but5clicked == true &&
but6clicked == true &&
but7clicked == true &&
but8clicked == true &&
but9clicked == true)
{
MessageBox.Show("You have tied! The game will now reset!");
ResetGame();
}
}
private void but1_Click(object sender, EventArgs e)
{
if (but1clicked == false)
{
if (Convert.ToBoolean(_clicks % 2))
{
but1.Text = "X";
but1clicked = true;
_clicks++;
}
else
{
but1.Text = "O";
but1clicked = true;
_clicks++;
}
}
else
MessageBox.Show("This box has already been chosen! Please choose another!");
Check();
Tie();
}
There are 9 buttons, just the same as the method but1_click. I didn’t think it was necessary to add them, though.
Solution
The buttons should be an array, if so you will replace btn with buttons[1]. Using the array will allow replacing this:
if (but1clicked == true &&
but2clicked == true &&
but3clicked == true &&
but4clicked == true &&
but5clicked == true &&
but6clicked == true &&
but7clicked == true &&
but8clicked == true &&
but9clicked == true)
With this:
if (buttons.All(button => button.clicked))
I assume that each button should have a clicked method. You will need some more modifications for this to work.
About the win lose checks, I suggest
triples = ( (0, 1, 2), (3, 4, 5) ..)
def wins(symbol) {
return triples.Any(triple => triple.All(index => buttons[index] == symbol))
}
All of this is pseudocode, but should be implementable with no problems.
There is much repetition in your code. While it is natural to think that way as a human, computer has a much stronger computing and memorization power than we do which we should take advantage of.
Click handler
Based on the naming, you have 9 of these click handlers with pretty the same code with only a single variation in the button’s name. You can write this in a more generic way, so that all the 9 buttons share the same click handler.
private void but1_Click(object sender, EventArgs e)`
The flow isn’t perfect either, as you should not do the end-game condition checking when the button is already being marked.
private void but1_Click(object sender, EventArgs e)
{
if (but1clicked == false)
{
// mark it with X or O
}
else
{
// show it is already marked
}
Check();
Tie();
}
It is unnecessary to check for both of the end-game conditions.
Here is how I would write it:
void Button_OnClick(object sender, EventArgs e)
{
// usually, the sender is the one that triggered the event.
// in this case, the button that is being clicked.
var button = sender as Button;
if (!string.IsNullOrEmpty(button.Text))
{
MessageBox.Show("This box has already been chosen! Please choose another!");
return;
}
button.Text = (checkedCount % 2 == 0)? "X" : "O";
checkedCount++;
// *I'll come back to these in the next section
Check();
Tie();
}
Logic flows
Check and Tie seems to be a bit confusing here. Usually, you want to use verb to for an action. However, you are not checking a box or making the game a tie in these methods, but rather just to evaluation the condition.
Also, you are resetting the game inside these method which adds a layer of complexity and surprise(you really don’t want this) to it.
I have rewritten these two to return a boolean and moved ResetGame outside of it. Now the logic of the game is more clear :
void Button_OnClick(object sender, EventArgs e)
{
var button = sender as Button;
if (!string.IsNullOrEmpty(button.Text))
{
MessageBox.Show("This box has already been chosen! Please choose another!");
return;
}
button.Text = (checkedCount % 2 == 0)? "X" : "O";
checkedCount++;
if (AnySideWins() || IsTie())
ResetGame();
}
Winning Condition[AnySideWins]
There is 3 horizontal, 3 vertical and 2 diagonal lines, a total of 8, which you can win by filling up any of them. This times both sides, and you have 16 conditions to check.
And if you decide to change anything by any chance, that is 16 edits there.
Let’s ask the question : What exactly are we doing here?
We want to check if any of these line has been filled by the same mark. In other words, we want to check if any of Button[3] contains the same text thrice and are not empty.
bool AnySideWins()
{
var horizontals = new[]
{
new [] { but1, but2, but3 },
new [] { but4, but5, but6 },
new [] { but7, but8, but9 },
};
var verticals = new[]
{
new [] { but1, but4, but7 },
new [] { but2, but5, but8 },
new [] { but3, but6, but9 },
};
var diagonals = new[]
{
new [] { but1, but5, but9 },
new [] { but3, but5, but7 },
};
// with all the possible winning lines
var winningLine = horizontals.Concat(verticals).Concat(diagonals)
// find the first line that
.FirstOrDefault(buttons =>
// whose buttons' text are all the same
buttons
// take each button.text
.Select(button => button.Text)
// exclude unchecked ones
.Where(text => !string.IsNullOrEmpty(text))
// group them by themselves, so X goes togather and O goes togather
.GroupBy(text => text)
// and see if any group has 3 elements.
.Any(group => group.Count() == 3));
// *Note: if FirstOrDefault doesnt find anything, the default of Button[], which is null, will be returned
if (winningLine != null)
MessageBox.Show("Game Over! " + winningLine.First().Text + " Wins!");
return winningLine != null;
}
Winning Condition[IsTie]
Same thing as above, but much simpler : We want to check if all buttons have been marked.
bool IsTie()
{
var buttons = new[] { but1, but2, but3, but4, but5, but6, but7, but8, but9 };
// if all button are marked then it is a tie.
// or if we flip the previous assertion,
// if any button is not marked, then it is not a tie
// finally we negate the result to match the method name
var tied = !buttons.Any(button => string.IsNullOrEmpty(button.Text));
if (tied)
MessageBox.Show("You have tied! The game will now reset!");
return tied;
}
Game Reset
void ResetGame()
{
var buttons = new[] { but1, but2, but3, but4, but5, but6, but7, but8, but9 };
foreach (var button in buttons)
button.Text = string.Empty;
checkedCount = 0;
}
Putting it all togather
I’ve rewritten this in LinqPad, for quick testing : source.linq
Button but1,but2,but3,but4,but5,but6,but7,but8,but9;
byte checkedCount = 0;
void Main()
{
var board = new TableLayoutPanel();
board.ColumnCount = 3;
board.RowCount = 3;
but1 = new Button();
but2 = new Button();
but3 = new Button();
but4 = new Button();
but5 = new Button();
but6 = new Button();
but7 = new Button();
but8 = new Button();
but9 = new Button();
board.Controls.Add(but1, 0, 0);
board.Controls.Add(but2, 0, 1);
board.Controls.Add(but3, 0, 2);
board.Controls.Add(but4, 1, 0);
board.Controls.Add(but5, 1, 1);
board.Controls.Add(but6, 1, 2);
board.Controls.Add(but7, 2, 0);
board.Controls.Add(but8, 2, 1);
board.Controls.Add(but9, 2, 2);
foreach (var button in board.Controls.OfType<Button>())
{
(button as Button).Click += Button_OnClick;
}
board.Dump();
}
// Define other methods and classes here
void Button_OnClick(object sender, EventArgs e)
{
var button = sender as Button;
if (!string.IsNullOrEmpty(button.Text))
{
MessageBox.Show("This box has already been chosen! Please choose another!");
return;
}
button.Text = (checkedCount % 2 == 0)? "X" : "O";
checkedCount++;
if (AnySideWins() || IsTie())
ResetGame();
}
bool AnySideWins()
{
var horizontals = new[]
{
new [] { but1, but2, but3 },
new [] { but4, but5, but6 },
new [] { but7, but8, but9 },
};
var verticals = new[]
{
new [] { but1, but4, but7 },
new [] { but2, but5, but8 },
new [] { but3, but6, but9 },
};
var diagonals = new[]
{
new [] { but1, but5, but9 },
new [] { but3, but5, but7 },
};
// with all the possible winning lines
var winningLine = horizontals.Concat(verticals).Concat(diagonals)
// find the first line that
.FirstOrDefault(buttons =>
// whose buttons' text are all the same
buttons
// take each button.text
.Select(button => button.Text)
// exclude unchecked ones
.Where(text => !string.IsNullOrEmpty(text))
// group them by themselves, so X goes togather and O goes togather
.GroupBy(text => text)
// and see if any group has 3 elements.
.Any(group => group.Count() == 3));
// *Note: if FirstOrDefault doesnt find anything, the default of Button[], which is null, will be returned
if (winningLine != null)
MessageBox.Show("Game Over! " + winningLine.First().Text + " Wins!");
return winningLine != null;
}
bool IsTie()
{
var buttons = new[] { but1, but2, but3, but4, but5, but6, but7, but8, but9 };
// if all button are marked then it is a tie.
// or if we flip the previous assertion,
// if any button is not marked, then it is not a tie
// finally we negate the result to match the method name
var tied = !buttons.Any(button => string.IsNullOrEmpty(button.Text));
if (tied)
MessageBox.Show("You have tied! The game will now reset!");
return tied;
}
void ResetGame()
{
var buttons = new[] { but1, but2, but3, but4, but5, but6, but7, but8, but9 };
foreach (var button in buttons)
button.Text = string.Empty;
checkedCount = 0;
}
Note: I didn’t replace but{1-9} with array, as it would make the code incompatible with what OP currently has.
Here is the extra bit :
Button[] cells = new Button[9];
byte checkedCount = 0;
void Main()
{
var board = new TableLayoutPanel();
board.ColumnCount = 3;
board.RowCount = 3;
for (int i = 0; i < 9; i++)
{
cells[i] = new Button();
cells[i].Click += Button_OnClick;
cells[i].Text = i.ToString();
board.Controls.Add(cells[i], i / 3, i % 3);
}
board.Dump();
}
Finally run the source code through this :
Regex.Replace(sourceCode, @"but(?<id>d)", m => $"buttons[{ int.Parse(m.Groups["id"].Value) - 1 }]")