1
\$\begingroup\$

I have written a simple Tic Tac Toe game in WPC C#. I am just a beginner and learning. Could you tell me if there is something wrong with my code / could be done better? I am pretty much satisfied with how it works, but I´d like to know if I made some things, that could have been done better. Thanks in advance.

In the window there are 9 buttons, 1 reset button, and a textBlock for messages.

MainWindow.xaml.cs Class

namespace TicTacToe
{
    public partial class MainWindow : Window
    {
        private Button[] buttons;
        private TextBlock textAlert;
        private Button resetButt;
        private TicTacToeGame game;


        public MainWindow()
        {
            InitializeComponent();

            // sends array of buttons, alertTextBlock and resetButton to TicTacToe game
            textAlert = AlertTextBlock;
            buttons = new Button[9];

            buttons[0] = button1x1;
            buttons[1] = button1x2;
            buttons[2] = button1x3;
            buttons[3] = button2x1;
            buttons[4] = button2x2;
            buttons[5] = button2x3;
            buttons[6] = button3x1;
            buttons[7] = button3x2;
            buttons[8] = button3x3;
            resetButt = resetButton;


            game = new TicTacToeGame(buttons, textAlert, resetButt);
        }

        private void button_Click(object sender, RoutedEventArgs e)
        {

            Button clicked = (Button)sender;    //clicked button
            if (game.getPlayer1Turn())      
            {
                clicked.Content = "X";  //sets button to X
            }
            else
            {
                clicked.Content = "O";  //sets button to O
            }
            clicked.IsEnabled = false;  //disables clicked button, so it doesn´t change

            game.nextTurn();    //proceeds to next turn
        }

        private void resetButton_click(object sender, RoutedEventArgs e)
        {
            game.reset(); 
        }
    }
}

TicTacToe.cs class

namespace TicTacToe
{
    class TicTacToeGame
    {
        private bool player1Turn;
        private int turn;
        private TextBlock alertText;
        private Button[] buttons;
        private Button resetButton;
        private bool gameFinished;
        private int[][] winningPos = new int[8][]
            {
            new int[3] {0,1,2},
            new int[3] {3,4,5},
            new int[3] {6,7,8},
            new int[3] {0,3,6},
            new int[3] {1,4,7},
            new int[3] {2,5,8},
            new int[3] {0,4,8},
            new int[3] {2,4,6}
            };

        /* constructor */
        public TicTacToeGame(Button[] buttons, TextBlock alertText, Button resetButton)
        {
            player1Turn = false;
            turn = 0;
            this.alertText = alertText;
            this.buttons = buttons;
            this.resetButton = resetButton;
            gameFinished = false;
            nextTurn();
        }

        /* tells who is on turn true - player1; false - player2*/
        public bool getPlayer1Turn()
        {
            return player1Turn;
        }

        /* method nextTurn(), pretty straight forward */
        public void nextTurn()
        {
            player1Turn = !player1Turn;
            turn++;
            if(player1Turn)
                alertText.Text = "It is player 1´s turn. (X). Turn: " + turn;
            else
                alertText.Text = "It is player 2´s turn. (O) Turn: " + turn;
            Validate();
        }


        /* Check if one of the player won */
        private void Validate()
        {
            string content = "";    
            string winner = "";
            foreach (int[] p in winningPos)
            {
                for (int i = 0; i <= 2; i++)
                {
                    if(i == 0)
                       content = buttons[p[i]].Content.ToString();
                    if (content != buttons[p[i]].Content.ToString())
                        break;
                    if (content == "")
                        break;
                    if (i == 2 && content == buttons[p[i]].Content.ToString())
                    {
                        gameFinished = true;
                        winner = content;
                    }
                }
            }
            if (gameFinished)
            { 
                if(winner == "X")
                    alertText.Text = "Player 1 wins! Congratulations!";
                else if (winner == "O")
                    alertText.Text = "Player 2 wins! Congratulations!";
                disableAll();
                resetButton.IsEnabled = true;
            }
            else if (turn == 10 && !gameFinished)
            {
                alertText.Text = "Tie!";
                disableAll();
                resetButton.IsEnabled = true;
            }
        }

        /* disables all the buttons (when one of the player wins) */
        private void disableAll()
        {
            foreach (Button butt in buttons)
                butt.IsEnabled = false;
        }


        /* method to reset the game, allowing to play again */
        public void reset()
        {
            foreach (Button butt in buttons)
            {
                butt.IsEnabled = true;
                butt.Content = "";
            }
            player1Turn = false;
            turn = 0;
            gameFinished = false;
            resetButton.IsEnabled = false;
            nextTurn();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ A fun addition would be a one-player mode - something I did a while back and highly recommend! \$\endgroup\$ – user3791372 May 7 '16 at 9:55
3
\$\begingroup\$

1 . Method names should be in Pascal case typing or at least that's the convention i.e SomeMethod() instead of someMethod() you have just 1 method that start's with capital letter the others dont which makes your naming inconsitent.

2 . Validate is a confusing name .net already contains method with such name.

3.Magic numbers should be avoided and you have some of them (0,2,10). It's really unclear what they are there for.

4 . If statements can be refactored into functions in order to bring more readability. I'm looking at those specifically

                if(i == 0)
                   content = buttons[p[i]].Content.ToString();
                if (content != buttons[p[i]].Content.ToString())
                    break;
                if (content == "")
                    break;
                if (i == 2 && content == buttons[p[i]].Content.ToString())
                {
                    gameFinished = true;
                    winner = content;
                }

5 . if (game.getPlayer1Turn()) You might want to change the name of that to IsPlayer1Turn.

6 . The entire function Validate can be refactored into a few smaller ones - one for Tie, one for the already determined winner and this :

        foreach (int[] p in winningPos)
        {
            for (int i = 0; i <= 2; i++)
            {
                if(i == 0)
                   content = buttons[p[i]].Content.ToString();
                if (content != buttons[p[i]].Content.ToString())
                    break;
                if (content == "")
                    break;
                if (i == 2 && content == buttons[p[i]].Content.ToString())
                {
                    gameFinished = true;
                    winner = content;
                }
            }
        }

can become another function that will return the content gamefinished wont be needed I think in this case because if your functions should basically return the content and if it doesn't find occurrence return string.Empty in which case you can check if(string.IsNullOrEmpty(winner)) that means we don't have a winner yet. IF you still need the gameFinished variable you can edit it like this. But as I said this method still needs some const variables and some proper refactoring of those if statements. Checking for a winner can be done in short function too :

private bool FoundWinner(string winner) => string.IsNullOrEmpty(winner);


if (FoundWinner)
{ 
      if(winner == "X")
           alertText.Text = "Player 1 wins! Congratulations!";
      else if (winner == "O")
           alertText.Text = "Player 2 wins! Congratulations!";
      disableAll();
      resetButton.IsEnabled = true;
 }

can easily become

        if (FoundWinner)
        {
            alertText.Text = winner == "X" ? "Player 1 wins! Congratulations!" : "Player 2 wins! Congratulations!";
            disableAll();
            resetButton.IsEnabled = true;
        }

You can do something similar for the Tie checking.

7 . Use braces { } even one line if statements should have those it brings more readability to your code.

One last addition, that's something you might want to do (not necessary).

        buttons = new Button[9];

        buttons[0] = button1x1;
        buttons[1] = button1x2;
        buttons[2] = button1x3;
        buttons[3] = button2x1;
        buttons[4] = button2x2;
        buttons[5] = button2x3;
        buttons[6] = button3x1;
        buttons[7] = button3x2;
        buttons[8] = button3x3;

You already have an array of buttons. Just create everything during the runtime set the Location,Size, and all the other properties of a button you need from the code behind.

\$\endgroup\$
  • \$\begingroup\$ Thanks! Could you give me an example of what you mean in point 4.? I don´t get how could it be refactored. \$\endgroup\$ – Marinaro May 7 '16 at 10:39
  • \$\begingroup\$ I'm honestly not sure what they are checking for. See how FoundWinner is done. You should do them in a similar if not the exact same way. It uses body expression it is equivalent to private bool FoundWinner(string winner) { return string.IsNullOrEmpty(winner); } The idea is to have respective names so you can easily say what this chunk of code is doing. private bool VariableEquals0(int input) => input == 0 something like this but with a better name. Use : if(VariableEquals0(i)) \$\endgroup\$ – Denis May 7 '16 at 10:41
2
\$\begingroup\$

I like that you separated the business logic from the GUI. The most beginners (including me a few years ago) put all the code into the windows class.

However, your business class knows most of the GUI elements which should be avoided if possible. Instead of using the boolean "player1turn" consider to use an enum (Player1, Player2) and the variable "currentPlayer" wich would be more readable.

In WPF there is a pattern called MVVM (Model-View-ViewModel) which is quasi-standard. For such a simple application like your TicTacToe game, your solution may be acceptable. But if you ask "what could be done better for WPF application in general?", I would suggest that you take a look at the MVVM pattern and see if you can redesign your code implementing it.

\$\endgroup\$
  • \$\begingroup\$ Disclaimer - using MVVM complicates things very quickly and is best to use a framework otherwise you're re-writing boiler plate code! As stated above, for newbies, MVVM is best to be avoided for a while. \$\endgroup\$ – user3791372 May 7 '16 at 9:52

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.