3
\$\begingroup\$

I had to build a 2-player Tic Tac Toe app in C# w/ WinForms for a pre-interview screen. I got past that stage successfully and the app works flawlessly, but I would like to get some feedback.

Although I've been programming C# for a while, I don't have too much experience with certain architectural paradigms such as Model-Viewer-Presenter. In a bid to learn more, I built the Tic Tac Toe app following the MVP Pattern.

GitHub

Program.cs

using System;
using System.Windows.Forms;
using TicTacToe.Model;
using TicTacToe.Presenter;

namespace TicTacToe
{
    static class Program
    {
        /// <summary>
        /// The main entry point for the application.
        /// Creates a new view and model and wires them with a presenter.
        /// </summary>
        [STAThread]
        static void Main()
        {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);

            TicTacToeForm ticTacToeForm = new TicTacToeForm();
            GamePresenter presenter = new GamePresenter(ticTacToeForm, new GridModel());

            Application.Run(ticTacToeForm);
        }
    }
}

GridModel.cs Should potentially be called GameModel.cs instead?

namespace TicTacToe.Model
{
    class GridModel : IGridModel
    {
        private char[,] grid;
        private bool playerOneTurn;

        /// <summary>
        /// Gets and sets the boolean indicating whether it's the first players turn.
        /// </summary>
        public bool PlayerOneTurn 
        {
            get { return playerOneTurn; }
            set { playerOneTurn = value; }
        }

        public GridModel()
        {
            grid = new char[3, 3];
            playerOneTurn = true;
        }

        /// <summary>
        /// Creates a new empty game grid and gives the turn to the first player.
        /// </summary>
        public void NewGrid()
        {
            grid = new char[3, 3];
            playerOneTurn = true;
        }

        /// <summary>
        /// Sets the player's marker on the specified grid square.
        /// </summary>
        /// <param name="piece">Character representing the players marker. Should be 'X' or 'O'.</param>
        /// <param name="row">The row of the square that the marker should be placed on.</param>
        /// <param name="col">The column of the square that the marker should be placed on.</param>
        public void SetPiece(char piece, int row, int col)
        {
            grid[row, col] = piece;
        }

        /// <summary>
        /// Gets the player's marker on the specified grid square.
        /// </summary>
        /// <param name="row">The row of the square in question.</param>
        /// <param name="col">The column of the square in question.</param>
        /// <returns>The character on the specified grid square.</returns>
        public char GetPiece(int row, int col)
        {
            return grid[row, col];
        }
    }
}

GamePresenter.cs

using System.Drawing;
using System.Windows.Forms;
using TicTacToe.Model;

namespace TicTacToe.Presenter
{
    public class GamePresenter
    {
        private readonly ITicTacToeForm view;
        private readonly IGridModel model;

        public GamePresenter(ITicTacToeForm view, IGridModel model)
        {
            this.view = view;
            this.model = model;

            this.view.Presenter = this;
        }

        /// <summary>
        /// Creates a new empty grid in the model and resets the view controls.
        /// </summary>
        internal void NewGame()
        {
            model.NewGrid();
            var gridPanel = view.ButtonGrid;

            foreach (Button b in gridPanel.Controls)
            {
                int row = gridPanel.GetRow(b);
                int col = gridPanel.GetColumn(b);

                b.Text = model.GetPiece(row, col) + "";
                b.ForeColor = SystemColors.ControlText;
                b.BackColor = SystemColors.Window;
                b.UseVisualStyleBackColor = true;
            }
            gridPanel.Enabled = true;
        }

        /// <summary>
        /// Displays a temporary marker on the board at the specified cell.
        /// </summary>
        /// <param name="row">The row of the cell to highlight.</param>
        /// <param name="col">The column of the cell to highlight.</param>
        internal void ShowTemporaryPlayerChar(int row, int col)
        {
            char c = model.GetPiece(row, col);
            var gridPanel = view.ButtonGrid;

            if (c == '\0')
            {
                Button b = (Button)gridPanel.GetControlFromPosition(col, row);
                if (model.PlayerOneTurn)
                {
                    b.ForeColor = Color.LightBlue;
                    b.Text = "X";
                }
                else
                {
                    b.ForeColor = Color.LightCoral;
                    b.Text = "O";
                }
            }
        }

        /// <summary>
        /// Hides the temporary marker on the board at the specified cell.
        /// </summary>
        /// <param name="row">The row of the cell to no longer highlight.</param>
        /// <param name="col">The column of the cell to no longer highlight.</param>
        internal void HideTemporaryPlayerChar(int row, int col)
        {
            char c = model.GetPiece(row, col);
            var gridPanel = view.ButtonGrid;

            if (c == '\0')
            {
                Button b = (Button)gridPanel.GetControlFromPosition(col, row);
                b.Text = "";
            }
        }

        /// <summary>
        /// Plays a turn, which consists of placing the user's marker in the model and on the view at the desired spot (if possible), informing the model that the next turn is starting and determining if there are any wins or ties.
        /// </summary>
        /// <param name="row">The row of the cell that the player wishes to place a marker on.</param>
        /// <param name="col">The column of the cell that the player wishes to place a marker on.</param>
        internal void PlayTurn(int row, int col)
        {
            if (model.GetPiece(row, col) != '\0')
                return;

            char c = model.PlayerOneTurn ? 'X' : 'O';
            var gridPanel = view.ButtonGrid;

            model.SetPiece(c, row, col);

            Button b = (Button)gridPanel.GetControlFromPosition(col, row);
            if (model.PlayerOneTurn)
            {
                b.ForeColor = Color.Blue;
                b.Text = model.GetPiece(row, col) + "";
            }
            else
            {
                b.ForeColor = Color.Crimson;
                b.Text = model.GetPiece(row, col) + "";
            }
            model.PlayerOneTurn = !model.PlayerOneTurn;
            switch (IsWinOrTie())
            {
                case 'X':
                    MessageBox.Show("Congrats to Player X! Press 'Restart' to play a new round.", "X has won");
                    gridPanel.Enabled = false;
                    break;
                case 'O':
                    MessageBox.Show("Congrats to Player O! Press 'Restart' to play a new round.", "O has won");
                    gridPanel.Enabled = false;
                    break;
                case 't':
                    MessageBox.Show("It's a tie! Press 'Restart' to play a new round.", "Tie");
                    gridPanel.Enabled = false;
                    break;
                default:
                    break;
            }
        }

        /// <summary>
        /// Determines if there is a winner by checking the grid rows, columns and diagonals. If there is none, checks whether the game should continue, or that there is a tie.
        /// </summary>
        /// <returns>Returns a character indicating who the winner is, if there is one. Otherwise returns a 't' for tie or an '\\0' for no action.</returns>
        private char IsWinOrTie()
        {
            for (int i = 0; i < 3; i++)
            {
                if (model.GetPiece(i, 0) != '\0' && model.GetPiece(i, 0) == model.GetPiece(i, 1) && model.GetPiece(i, 1) == model.GetPiece(i, 2))
                {
                    HighlightWin(i, 0, i, 1, i, 2);
                    return model.GetPiece(i, 0);
                }
                if (model.GetPiece(0, i) != '\0' && model.GetPiece(0, i) == model.GetPiece(1, i) && model.GetPiece(1, i) == model.GetPiece(2, i))
                {
                    HighlightWin(0, i, 1, i, 2, i);
                    return model.GetPiece(0, i);
                }
            }
            if (model.GetPiece(0, 0) != '\0' && model.GetPiece(0, 0) == model.GetPiece(1, 1) && model.GetPiece(1, 1) == model.GetPiece(2, 2))
            {
                HighlightWin(0, 0, 1, 1, 2, 2);
                return model.GetPiece(1, 1);
            }
            if (model.GetPiece(0, 2) != '\0' && model.GetPiece(0, 2) == model.GetPiece(1, 1) && model.GetPiece(2, 0) == model.GetPiece(1, 1))
            {
                HighlightWin(0, 2, 1, 1, 2, 0);
                return model.GetPiece(1, 1);
            }
            for (int i = 0; i < 3; i++)
            {
                for (int j = 0; j < 3; j++)
                {
                    if (model.GetPiece(i, j) == '\0')
                        return '\0';
                }
            }
            return 't';
        }

        /// <summary>
        /// Highlights the boxes on the game board where the winning markers are situated.
        /// </summary>
        /// <param name="a">Row of the 1st button.</param>
        /// <param name="b">Column of the 1st button.</param>
        /// <param name="c">Row of the 2nd button.</param>
        /// <param name="d">Column of the 2nd button.</param>
        /// <param name="e">Row of the 3rd button.</param>
        /// <param name="f">Column of the 3rd button.</param>
        private void HighlightWin(int a, int b, int c, int d, int e, int f)
        {
            var gridPanel = view.ButtonGrid;

            Button b0 = (Button)gridPanel.GetControlFromPosition(b, a);
            Button b1 = (Button)gridPanel.GetControlFromPosition(d, c);
            Button b2 = (Button)gridPanel.GetControlFromPosition(f, e);

            Color color = model.PlayerOneTurn ? Color.LightCoral : Color.LightBlue;

            b0.BackColor = b1.BackColor = b2.BackColor = color;
        }
    }
}

TicTacToeForm.cs

using System;
using System.Collections.Generic;
using System.Windows.Forms;
using TicTacToe.Presenter;

namespace TicTacToe
{
    public partial class TicTacToeForm : Form, ITicTacToeForm
    {
        /// <summary>
        /// Initializes the game and sets event handlers for each button.
        /// Additionally, displays a short instruction message to the players.
        /// </summary>
        public TicTacToeForm()
        {
            InitializeComponent();
            foreach (Button b in this.gridPanel.Controls)
            {
                b.MouseEnter += B_MouseEnter;
                b.MouseLeave += B_MouseLeave;
                b.MouseClick += B_MouseClick;
            }
            MessageBox.Show("1. Click any square to place a piece\n2. Take turns with a friend\n3. Have fun! ", "How to play");
        }

        /// <summary>
        /// Gets and sets the presenter for the view.
        /// </summary>
        public GamePresenter Presenter { private get; set; }

        /// <summary>
        /// Gets the TableLayoutPanel containing the game board's squares (buttons).
        /// </summary>
        public TableLayoutPanel ButtonGrid
        {
            get { return gridPanel; }
        }

        /// <summary>
        /// Event handler for when the player's mouse moves into a game square.
        /// Temporarily displays the player's marker on the square with reduced opacity.
        /// </summary>
        /// <param name="sender"></param>
        /// <param name="e"></param>
        private void B_MouseEnter(object sender, EventArgs e)
        {
            Button button = sender as Button;

            int row = gridPanel.GetRow(button);
            int col = gridPanel.GetColumn(button);
            Presenter.ShowTemporaryPlayerChar(row, col);

        }

        /// <summary>
        /// Event handler for when the player's mouse moves out of a game square.
        /// Removes the temporary marker that was previously displayed.
        /// </summary>
        /// <param name="sender"></param>
        /// <param name="e"></param>
        private void B_MouseLeave(object sender, EventArgs e)
        {
            Button button = sender as Button;

            int row = gridPanel.GetRow(button);
            int col = gridPanel.GetColumn(button);
            Presenter.HideTemporaryPlayerChar(row, col);

        }

        /// <summary>
        /// Attempts to place a marker on the specified square. 
        /// No action taken if the square is already filled.
        /// Otherwise hands the turn to the other player.
        /// </summary>
        /// <param name="sender"></param>
        /// <param name="e"></param>
        private void B_MouseClick(object sender, MouseEventArgs e)
        {
            Button button = sender as Button;

            int row = gridPanel.GetRow(button);
            int col = gridPanel.GetColumn(button);
            Presenter.PlayTurn(row, col);
        }

        /// <summary>
        /// Displays a message box with the author and license information.
        /// </summary>
        /// <param name="sender"></param>
        /// <param name="e"></param>
        private void aboutToolStripMenuItem_Click(object sender, EventArgs e)
        {
            MessageBox.Show("Created by Mansib Rahman on 12/13/2016.\n\nLicensed under the MIT License. License text available under LICENSE.", "About");
        }

        /// <summary>
        /// Resets the board and starts a new game.
        /// </summary>
        /// <param name="sender"></param>
        /// <param name="e"></param>
        private void newGameToolStripMenuItem_Click(object sender, EventArgs e)
        {
            Presenter.NewGame();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ What version of .Net are you targeting? \$\endgroup\$ – RubberDuck Dec 16 '16 at 2:59
  • \$\begingroup\$ Also, would you mind putting the auto-gen form code in a gist? I'd kind of like to build your code and run your code. Makes it easier to review. (Don't feel obligated to, this is already a great question!) \$\endgroup\$ – RubberDuck Dec 16 '16 at 3:07
  • 1
    \$\begingroup\$ 4.5.2. And sure thing, here you go: gist.github.com/gaessaki/aff6fb937d2ca8f6aac55485f705374c. Let me know if you would like me to upload the solution or installer somewhere directly. Thanks! \$\endgroup\$ – Gaessaki Dec 16 '16 at 14:50
2
\$\begingroup\$
  1. I think, your Presenter is doing too much. The logic about who wins, the next move, etc. should go to the model. The logic about highlighting, colors, X and O's constants, message strings etc. belong to the View.

  2. You are appending an empty string to a char for the side-effect of converting to a string:

    b.Text = model.GetPiece(row, col) + "";

Don't do that. Use the ToString() function on chars.

  1. Avoid repeating string literals like '\0', 'X', etc. all the time. Use string constants:

    const char EMPTY_SQUARE = '\0';

  2. Don't duplicate your code. Try to identify repeating patterns and extract them either in small functions, or code more cleverly.

            switch (IsWinOrTie())
        {
            case 'X':
                MessageBox.Show("Congrats to Player X! Press 'Restart' to play a new round.", "X has won");
                gridPanel.Enabled = false;
                break;
            case 'O':
                MessageBox.Show("Congrats to Player O! Press 'Restart' to play a new round.", "O has won");
                gridPanel.Enabled = false;
                break;
            case 't':
                MessageBox.Show("It's a tie! Press 'Restart' to play a new round.", "Tie");
                gridPanel.Enabled = false;
                break;
            default:
                break;
        }
    

Every case in the switch does the same thing. Only the contents of the message box change.

Check the method IsWinOrTie() in the Presenter. (Btw, Good Lord! What a tsunami of numbers...)

The pattern is: A "selection" of 3 squares in the board (being either a row, a column, or a diagonal). We want to do something with that (check if the contents belong to the same player, highlight, etc.)

There could be e.g. an enumeration describing the selection and methods returning the corresponding 3 squares. Then IsWinOrTie() could simply do a loop on enumerations and check in a one liner:

squareA != EMPTY_SQUARE && squareA == squareB && squareB == squareB && squareB == squareC.
  1. The method HighlightWin() takes 6 parameters. This is terrible. Consider creating a struct:

    struct Position { public int row; public int col; }

You can pass an array of such structs instead.

\$\endgroup\$

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.