12
\$\begingroup\$

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;
    }
}
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Stylistic point - try to give your buttons and objects a good name, this could be: btnPlace, tbPlayerOneName etc \$\endgroup\$ – SomeRandomPerson Nov 13 '15 at 17:00
  • \$\begingroup\$ Food for thought: how would you go about refactoring your code to expand the game to Ultimate Tic Tac Toe? \$\endgroup\$ – Mathieu Guindon Nov 13 '15 at 17:25
  • \$\begingroup\$ @Mat's Mug, oh man thinking about the code I write, that would make it look terrifying.... \$\endgroup\$ – Tom L Nov 13 '15 at 19:51
  • \$\begingroup\$ It's easier than you think, Eric Lippert's answer explains how you could achieve that :-) \$\endgroup\$ – Mathieu Guindon Nov 14 '15 at 16:33
  • 1
    \$\begingroup\$ If you're just getting into C# I'd recommend to not bother with Winforms and use something more modern like WPF instead. \$\endgroup\$ – Roman Reiner Nov 14 '15 at 16:34
16
\$\begingroup\$
//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 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.

\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Tom L Nov 13 '15 at 19:53
  • \$\begingroup\$ 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. \$\endgroup\$ – Tom L Nov 14 '15 at 2:38
  • \$\begingroup\$ 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. \$\endgroup\$ – Tom L Nov 14 '15 at 2:58
  • \$\begingroup\$ 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. \$\endgroup\$ – Brian Nov 16 '15 at 14:14
9
\$\begingroup\$

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 "".

\$\endgroup\$
  • \$\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 Nov 13 '15 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 Nov 13 '15 at 20:35
  • \$\begingroup\$ The methods all already accept the button as a parameter (the sender). \$\endgroup\$ – BlueRaja - Danny Pflughoeft Nov 13 '15 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 Nov 14 '15 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 Nov 14 '15 at 8:27
8
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Brian Nov 16 '15 at 14:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.