Tic Tac Toe game in C#

Question

I wanted to get into C#, so I tried making a simple game. My first language was Java, so it was pretty easy to get the hang of C#.

I know that there are multiple other posts of Tic Tac Toe games, but I want some critique on my own code. Is there any cleaner way? Any redundant code? Parts that I can jam together?

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace TicTacToe
{
public partial class Form1 : Form
{
    //Variable to store player, 0 is X, 1 is O.
    int counter = 0;
    public Form1()
    {
        InitializeComponent();
    }

    private void button1_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if(counter == 0)
        {
            button1.Text = "X";
            counter++;
        }else if(counter == 1)
        {
            button1.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button1.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    private void button2_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if (counter == 0)
        {
            button2.Text = "X";
            counter++;
        }
        else if (counter == 1)
        {
            button2.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button2.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    private void button3_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if (counter == 0)
        {
            button3.Text = "X";
            counter++;
        }
        else if (counter == 1)
        {
            button3.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button3.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    private void button4_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if (counter == 0)
        {
            button4.Text = "X";
            counter++;
        }
        else if (counter == 1)
        {
            button4.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button4.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    private void button5_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if (counter == 0)
        {
            button5.Text = "X";
            counter++;
        }
        else if (counter == 1)
        {
            button5.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button5.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    private void button6_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if (counter == 0)
        {
            button6.Text = "X";
            counter++;
        }
        else if (counter == 1)
        {
            button6.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button6.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    private void button7_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if (counter == 0)
        {
            button7.Text = "X";
            counter++;
        }
        else if (counter == 1)
        {
            button7.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button7.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    private void button8_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if (counter == 0)
        {
            button8.Text = "X";
            counter++;
        }
        else if (counter == 1)
        {
            button8.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button8.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    private void button9_Click(object sender, EventArgs e)
    {
        //Check who's turn it is
        if (counter == 0)
        {
            button9.Text = "X";
            counter++;
        }
        else if (counter == 1)
        {
            button9.Text = "O";
            counter--;
        }
        //Disable button so it cannot be changed
        button9.Enabled = false;

        //Check if anyone won, lose, tie
        check();
    }

    void check()
    {
        //Check if tie
        if (button1.Text != "" && button2.Text != "" && button3.Text != "" &&
            button4.Text != "" && button5.Text != "" && button6.Text != "" &&
            button7.Text != "" && button8.Text != "" && button9.Text != "")
        {
            textBox1.Text = "Tied";
        }

        //Check diagonal for X
        if (button1.Text == "X" && button5.Text == "X" && button9.Text == "X")
        {
            textBox1.Text = "Player X wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if(button3.Text == "X" && button5.Text == "X" && button7.Text == "X")
        {
            textBox1.Text = "Player X wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        //Check rows for X
        if(button1.Text == "X" && button2.Text == "X" && button3.Text == "X")
        {
            textBox1.Text = "Player X wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button4.Text == "X" && button5.Text == "X" && button6.Text == "X")
        {
            textBox1.Text = "Player X wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button7.Text == "X" && button8.Text == "X" && button9.Text == "X")
        {
            textBox1.Text = "Player X wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        //Check columns for X
        if(button1.Text == "X" && button4.Text == "X" && button7.Text == "X")
        {
            textBox1.Text = "Player X wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button2.Text == "X" && button5.Text == "X" && button8.Text == "X")
        {
            textBox1.Text = "Player X wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button3.Text == "X" && button6.Text == "X" && button9.Text == "X")
        {
            textBox1.Text = "Player X wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }

        //Check diagonal for O
        if (button1.Text == "O" && button5.Text == "O" && button9.Text == "O")
        {
            textBox1.Text = "Player O wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button3.Text == "O" && button5.Text == "O" && button7.Text == "O")
        {
            textBox1.Text = "Player O wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        //Check rows for O
        if (button1.Text == "O" && button2.Text == "O" && button3.Text == "O")
        {
            textBox1.Text = "Player O wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button4.Text == "O" && button5.Text == "O" && button6.Text == "O")
        {
            textBox1.Text = "Player O wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button7.Text == "O" && button8.Text == "O" && button9.Text == "O")
        {
            textBox1.Text = "Player O wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        //Check columns for O
        if (button1.Text == "O" && button4.Text == "O" && button7.Text == "O")
        {
            textBox1.Text = "Player O wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button2.Text == "O" && button5.Text == "O" && button8.Text == "O")
        {
            textBox1.Text = "Player O wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }
        if (button3.Text == "O" && button6.Text == "O" && button9.Text == "O")
        {
            textBox1.Text = "Player O wins";
            button1.Enabled = false;
            button2.Enabled = false;
            button3.Enabled = false;
            button4.Enabled = false;
            button5.Enabled = false;
            button6.Enabled = false;
            button7.Enabled = false;
            button8.Enabled = false;
            button9.Enabled = false;
        }

    }

    private void button10_Click(object sender, EventArgs e)
    {
        button1.Text = "";
        button1.Enabled = true;
        button2.Text = "";
        button2.Enabled = true;
        button3.Text = "";
        button3.Enabled = true;
        button4.Text = "";
        button4.Enabled = true;
        button5.Text = "";
        button5.Enabled = true;
        button6.Text = "";
        button6.Enabled = true;
        button7.Text = "";
        button7.Enabled = true;
        button8.Text = "";
        button8.Enabled = true;
        button9.Text = "";
        button9.Enabled = true;
        textBox1.Text = "";
        counter = 0;
    }
}
}

Stylistic point - try to give your buttons and objects a good name, this could be: btnPlace, tbPlayerOneName etc — Can O'Spam feels disassociated, Commented Nov 13, 2015 at 17:00
Food for thought: how would you go about refactoring your code to expand the game to Ultimate Tic Tac Toe? — Mathieu Guindon, Commented Nov 13, 2015 at 17:25
@Mat's Mug, oh man thinking about the code I write, that would make it look terrifying.... — Tom L, Commented Nov 13, 2015 at 19:51
It's easier than you think, Eric Lippert's answer explains how you could achieve that :-) — Mathieu Guindon, Commented Nov 14, 2015 at 16:33
If you're just getting into C# I'd recommend to not bother with Winforms and use something more modern like WPF instead. — Roman Reiner, Commented Nov 14, 2015 at 16:34

Mathieu Guindon · Accepted Answer · 2015-11-13 18:01:11Z

//Variable to store player, 0 is X, 1 is O.
int counter = 0;

Good thing that comment is there! This sets you up for some rather unimpressive player-turn logic though:

//Check who's turn it is
if(counter == 0)
{
    button1.Text = "X";
    counter++;
}else if(counter == 1)
{
    button1.Text = "O";
    counter--;
}

So who the current player is is stored in an int variable, that we increment and decrement.

How about this instead?

public enum Player
{
    X,
    O
}

Then you could store who the current player is in a Player variable:

//Variable to store player, X is X, O is O.
Player currentPlayer = Player.X;

Notice how useless that comment becomes with self-descriptive code?

Even better:

//Check who's turn it is
if(currentPlayer == Player.X)
{
    currentPlayer = Player.Y;
}
else if(currentPlayer == Player.Y) // notice '}' closing brace on its own line
{
    currentPlayer = Player.X;
}

Or, if you're into ternaries:

currentPlayer = currentPlayer == Player.X ? Player.Y : Player.X;

Quite a cut eh?

Now, you don't want to hard-code these button1 9 times. Coming from Java you might not be familiar with partial classes, but a winforms form is exactly that - and this InitializeComponent method being called in the constructor is located in another file that contains all the designer-generated code. Since I'm not seeing any event registration for your click handlers, I'm assuming you've set them up in the designer, and that the InitializeComponent method contains things like this:

button1.Click += button1_Click;
button2.Click += button2_Click;
button3.Click += button3_Click;
button4.Click += button4_Click;
button5.Click += button5_Click;
button6.Click += button6_Click;
button7.Click += button7_Click;
button8.Click += button8_Click;
button9.Click += button9_Click;

Instead of assigning handlers in the designer, get that code into your own constructor, and assign all buttons to the same handler:

button1.Click += Button_Click;
button2.Click += Button_Click;
button3.Click += Button_Click;
button4.Click += Button_Click;
button5.Click += Button_Click;
button6.Click += Button_Click;
button7.Click += Button_Click;
button8.Click += Button_Click;
button9.Click += Button_Click;

The Click event is an EventHandler delegate which has a signature taking arguments object sender and EventArgs e - the EventArgs is useless here, but the sender is the object that fired the click event - and that's your button, which means instead of referring to button1-button9, you can cast the sender to a Button and refer to that local variable now:

private void Button_Click(object sender, EventArgs e)
{
    var button = (Button)sender;
    button.Text = currentPlayer.ToString();
    currentPlayer = currentPlayer == Player.X ? Player.Y : Player.X;
    button.Enabled = false;
    Check(); // notice PascalCase for method name
}

The Check method is implicitly private - try to be explicit about access modifiers, otherwise it's not clear why every other private method says private, but not that one.

Oh man, that definitely looks a lot better.... I guess I got bad habits from learning Java from my professor..., thanks a lot, I'll try to implement that when I go to edit my code. — Tom L, Commented Nov 13, 2015 at 19:53
I tried figuring out the Button_Click class by myself, I just couldn't figure out how to get the source button! In Java, I would do e.getSource, I never would have guessed (Button)sender. — Tom L, Commented Nov 14, 2015 at 2:38
Seriously, thank you for all your detailed help, it really cut down my code a lot. What can I do about the Check method? Is there a way to disable buttons without spamming all those line of code? I thought about putting the Check method into the Button_Click method, but I want to see if there's another way first. — Tom L, Commented Nov 14, 2015 at 2:58
foreach (Button button in new[]{button1,button2,button3,button4,button5,button6,button7,button8,button9}){button.Enabled = false;} Move that code to its own private DisableAllButtons function, of course. — Brian, Commented Nov 16, 2015 at 14:14

BCdotWEB · Accepted Answer · 2015-11-13 16:26:50Z

10

Each of your buttonX_Click contains basically the same code. Don't copy-paste, instead create a method that contains this code and accepts the button as a parameter, and call that.

Same for your check() method: a lot of repeated and obviously copy-pasted code. Instead have a method where each of the buttons is either enabled or disabled, based on a boolean you pass as the parameter, that way you can also reduce the code of button10_Click.

Also, instead of setting each button, keep a collection of the buttons and loop though them.

It is customary to use string.Empty instead of "".

answered Nov 13, 2015 at 16:26

BCdotWEB

11.3k2 gold badges27 silver badges45 bronze badges

\$\begingroup\$ Oh, I wanted to make a loop for the buttons, but I use Windows Form Application, so I just double click a button and it generates that bit, but when I remove that part, it gives me an error in designer view. \$\endgroup\$
– Tom L
Commented Nov 13, 2015 at 19:52
\$\begingroup\$ @TomL instead of double clicking each button, you can make one event that all of the buttons point to. You can then handle which button actually was clicked by the sender argument in the event signature. \$\endgroup\$
– Michael McGriff
Commented Nov 13, 2015 at 20:35
\$\begingroup\$ The methods all already accept the button as a parameter (the sender). \$\endgroup\$
– BlueRaja - Danny Pflughoeft
Commented Nov 13, 2015 at 21:50
1

\$\begingroup\$ string.Empty vs String.Empty vs "" is rather arguable -- there is no clear consensus. However, there are certain situations where you must use one or the other: only "" works where a compile-time constant is required and only String.Empty (or string.Empty) works where compatibility with unmanaged code is required. \$\endgroup\$
– Bob
Commented Nov 14, 2015 at 3:10
1

\$\begingroup\$ @BCdotWEB Yes, I am aware that both sides have their reasons for using what they use. I'm just saying that it's not really an established convention amongst all C# programmers, and should not be presented as such. \$\endgroup\$
– Bob
Commented Nov 14, 2015 at 8:27

| Show 2 more comments

Eric Lippert · Accepted Answer · 2015-11-14 16:05:06Z

Other answers have noted that there is a great deal of unnecessary repetition in the button handling logic. There is also a great deal of repetition in the winning-condition-checking logic. Any time you copy-paste code like that, consider whether the code should go in a method which is then just called. There should be a "disable all buttons" method that you call, rather than repeating that logic a dozen times.

The winning condition logic appears to be incorrect. It seems to say that you cannot win on the last move, but only tie.

There is no variable keeping track of whether the game is won by x, won by o, tied, or still in progress. It's not necessary to have such a variable as it is redundant to the other game state, but sometimes it is easier to write the code when you can have one place to ask "is the game still in progress?"

The best advice I can give you though aside from eliminating all that redundancy is to separate your display logic from your game logic. You are using the appearance of some buttons as the storage for the game state, which means that you cannot change the appearance of the game without rewriting all of the logic. (Imagine for instance that you wanted to change the game so that instead of X and O it was blue and red buttons, or you wanted to have graphics on the buttons instead of text.)

The fancy term for this is "model view controller pattern", but you don't need to understand the formalism in order to use it effectively. Make a class that represents the game state and the rules; that class decides whether a move is legal, keeps track of the turns, and decides who won. The form class is responsible for taking in the moves and informing the game class.

Once you have that then many scenarios become easier. You can test the game logic independently of the UI, for example.

Other answers focus on DRY (don't repeat yourself) violations. Those code issues are definitely the low-hanging fruit in the code review and should thus be fixed first; clean code is easier to refactor. This answer goes a step beyond that into architectural improvements. Making these changes is more difficult, but they transition your code from being readable to being maintainable. — Brian, Commented Nov 16, 2015 at 14:29

Stack Exchange Network

Tic Tac Toe game in C#

3 Answers 3

Your Answer

Not the answer you're looking for? Browse other questions tagged
c#
tic-tac-toe
or ask your own question.

Linked

Hot Network Questions

Tic Tac Toe game in C#

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c#tic-tac-toe or ask your own question.

Linked

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
c#
tic-tac-toe
or ask your own question.