6
\$\begingroup\$

This is my first 'real' project in C# , i looked up some tutorials on the Internet for Tic Tac Toe. But they were not how i wanted it to be, so i came up with some own ideas and wanted some tips on improvement, because i know some things could be different. I simply don't really know how.

So here is my Code. You can try it on your own. You will need 2 Images for the X and O and upload them by yourself (size required to fit into the buttons 201x146 px)!
Also you will need to add the Visual Basic.dll as a Reference.

using System;
using System.IO;
using System.Resources;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using Microsoft.VisualBasic;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace TicTacToe
{
    public partial class Form1 : Form
    {
        Boolean player1 = true; //// player1's turn or not 

    Image x = Properties.Resources.x; /// Image for Player 1, in this case X 
    Image o = Properties.Resources.o; /// Image for Player 2, in this case O 

    string p1 = "Player 1"; /// default name for Player 1
    string p2 = "Player 2"; /// default name for Player 2

    int winp1 = 0;  /// score counter for Player 1 
    int winp2 = 0;  /// score counter for Player 2

    int drawcount = 0; /// counts how many field got clicked 
    Boolean won = false; /// state if someone won 

    public Form1()
    {
        InitializeComponent();
        this.Icon = Properties.Resources.icon; /* Icon for Form */


        p1 = Interaction.InputBox("Player 1","Playername",p1); /* Inputbox from VisualBasic to Enter a Name for Player 1 */
        if (p1 == "") p1 = "Player 1";

        p2 = Interaction.InputBox("Player 2","Playername",p2); /* Inputbox from VisualBasic to Enter a Name for Player 2 */
        if (p2 == "") p2 = "Player 2";

        turn.Text = p1;     /* shows text at the beginning whos turn it is, in this case Player 1 */
        p1winstxt.Text = p1;    /* sets the name for the score table for Player 1 */
        p2winstxt.Text = p2;    /* sets the name for the score table for Player 1 */

    }




    public void Playerstatus() /* checks whos turn it is */
    {
        if(player1 == true)
        {
            player1 = false;
            turn.Text = p2;
        }
        else
        {
            player1 = true;
            turn.Text = p1;
        }
        drawcount++;
        Checkwin();

    }

    public void Checkwin() /* checks if someone won, if not then check if all buttons have been clicked */
    {
        if(ul.Image == x && uc.Image == x && ur.Image == x)
        {
            P1win();
        }
        else if(ul.Image == o && uc.Image == o && ur.Image == o)
        {
            P2win();
        }
        /*-------------------------------------------------------------------------------*/
        if (ul.Image == x && cc.Image == x && br.Image == x)
        {
            P1win();
        }
        else if(ul.Image == o && cc.Image == o && br.Image == o)
        {
            P2win();
        }
        /*-------------------------------------------------------------------------------*/
        if (ul.Image == x && cl.Image == x && bl.Image == x)
        {
            P1win();
        }
        else if (ul.Image == o && cl.Image == o && bl.Image == o)
        {
            P2win();
        }
        /*-------------------------------------------------------------------------------*/
        if (uc.Image == x && cc.Image == x && bc.Image == x)
        {
            P1win();
        }
        else if(uc.Image == o && cc.Image == o && bc.Image == o)
        {
            P2win();
        }
        /*-------------------------------------------------------------------------------*/
        if (ur.Image == x && cr.Image == x && br.Image == x)
        {
            P1win();
        }
        else if(ur.Image == o && cr.Image == o && br.Image == o)
        {
            P2win();
        }
        /*-------------------------------------------------------------------------------*/
        if (ur.Image == x && cc.Image == x && bl.Image == x)
        {
            P1win();
        }
        else if(ur.Image == o && cc.Image == o && bl.Image == o)
        {
            P2win();
        }
        /*-------------------------------------------------------------------------------*/
        if (cr.Image == x && cc.Image == x && cl.Image == x)
        {
            P1win();
        }
        else if(cr.Image == o && cc.Image == o && cl.Image == o)
        {
            P2win();
        }
        /*-------------------------------------------------------------------------------*/
        if (bl.Image == x && bc.Image == x && br.Image == x)
        {
            P1win();
        }
        else if(bl.Image == o && bc.Image == o && br.Image == o)
        {
            P2win();
        }
        /*-------------------------------------------------------------------------------*/
        if (drawcount == 9) /* checks if all 9 buttons have been clicked */
        {
            if (won == false)
            {
                tie();
            }
        }

    }

    private void ol_Click(object sender, EventArgs e)
    {
        if(player1 == true)
        {
            ul.Image = x;
            Playerstatus();
        }
        else
        {
            ul.Image = o;
            Playerstatus();
        }

        ul.Enabled = false;
    }

    private void om_Click(object sender, EventArgs e)
    {
        if (player1 == true)
        {
            uc.Image = x;
            Playerstatus();
        }
        else
        {
            uc.Image = o;
            Playerstatus();
        }

        uc.Enabled = false;

    }

    private void or_Click(object sender, EventArgs e)
    {
        if (player1 == true)
        {
            ur.Image = x;
            Playerstatus();
        }
        else
        {
            ur.Image = o;
            Playerstatus();
        }

        ur.Enabled = false;
    }

    private void ml_Click(object sender, EventArgs e)
    {
        if (player1 == true)
        {
            cl.Image = x;
            Playerstatus();
        }
        else
        {
            cl.Image = o;
            Playerstatus();
        }

        cl.Enabled = false;
    }

    private void mm_Click(object sender, EventArgs e)
    {
        if (player1 == true)
        {
            cc.Image = x;
            Playerstatus();
        }
        else
        {
            cc.Image = o;
            Playerstatus();
        }

        cc.Enabled = false;
    }

    private void mr_Click(object sender, EventArgs e)
    {
        if (player1 == true)
        {
            cr.Image = x;
            Playerstatus();
        }
        else
        {
            cr.Image = o;
            Playerstatus();
        }

        cr.Enabled = false;
    }

    private void ul_Click(object sender, EventArgs e)
    {
        if (player1 == true)
        {
            bl.Image = x;
            Playerstatus();
        }
        else
        {
            bl.Image = o;
            Playerstatus();
        }

        bl.Enabled = false;
    }

    private void um_Click(object sender, EventArgs e)
    {
        if (player1 == true)
        {
            bc.Image = x;
            Playerstatus();
        }
        else
        {
            bc.Image = o;
            Playerstatus();
        }

        bc.Enabled = false;
    }

    private void ur_Click(object sender, EventArgs e)
    {
        if (player1 == true)
        {
            br.Image = x;
            Playerstatus();
        }
        else
        {
            br.Image = o;
            Playerstatus();
        }

        br.Enabled = false;
    }

    public void P1win() /* Player 1 won */
    {
        MessageBox.Show(p1 + " won!");
        foreach (Control c in Controls) /* disables all buttons */
        {
            Button b = c as Button;
            if (b != null)
            {
                b.Enabled = false;

            }
        }

        res.Enabled = true; /* reactivates the reset button */
        winp1++;
        drawcount = 0;
        p1wins.Text = winp1.ToString();
        won = true;

    }

    public void P2win() /* Player 2 won */
    {
        MessageBox.Show(p2 + " won!");
        foreach (Control c in Controls)
        {
            Button b = c as Button;
            if (b != null)
            {
                b.Enabled = false;

            }
        }

        res.Enabled = true;
        winp2++;
        p2wins.Text = winp2.ToString();
        drawcount = 0;
        won = true;
    }

    public void Resetgame() /* resets the game */
    {
        turn.Text = p1;
        player1 = true;

        foreach (Control c in Controls)
        {
            Button b = c as Button;
            if(b != null)
            {
                b.Enabled = true;
                b.Image = null;
            }
        }
    }

    private void res_Click(object sender, EventArgs e)
    {
        Resetgame();
    }

    public void tie() /* MessageBox for Tie */
    {
        MessageBox.Show("Tie!");
    }

    private void resetscore_Click(object sender, EventArgs e) /* resets the score */
    {
        winp1 = 0;
        winp2 = 0;

        p1wins.Text = winp1.ToString();
        p2wins.Text = winp1.ToString();
    }

  }
}

-- CONTENT OF Form1.Designer.cs --

 private void InitializeComponent()
        {
            this.ul = new System.Windows.Forms.Button();
            this.uc = new System.Windows.Forms.Button();
            this.ur = new System.Windows.Forms.Button();
            this.cl = new System.Windows.Forms.Button();
            this.cc = new System.Windows.Forms.Button();
            this.cr = new System.Windows.Forms.Button();
            this.bl = new System.Windows.Forms.Button();
            this.bc = new System.Windows.Forms.Button();
            this.turn = new System.Windows.Forms.Label();
            this.groupBox1 = new System.Windows.Forms.GroupBox();
            this.br = new System.Windows.Forms.Button();
            this.res = new System.Windows.Forms.Button();
            this.groupBox2 = new System.Windows.Forms.GroupBox();
            this.resetscore = new System.Windows.Forms.Button();
            this.p2wins = new System.Windows.Forms.Label();
            this.p1wins = new System.Windows.Forms.Label();
            this.p2winstxt = new System.Windows.Forms.Label();
            this.p1winstxt = new System.Windows.Forms.Label();
            this.groupBox1.SuspendLayout();
            this.groupBox2.SuspendLayout();
            this.SuspendLayout();
            // 
            // ul
            // 
            this.ul.Location = new System.Drawing.Point(29, 54);
            this.ul.Name = "ul";
            this.ul.Size = new System.Drawing.Size(201, 146);
            this.ul.TabIndex = 0;
            this.ul.TabStop = false;
            this.ul.UseVisualStyleBackColor = true;
            this.ul.Click += new System.EventHandler(this.ol_Click);
            // 
            // uc
            // 
            this.uc.Location = new System.Drawing.Point(226, 54);
            this.uc.Name = "uc";
            this.uc.Size = new System.Drawing.Size(201, 146);
            this.uc.TabIndex = 1;
            this.uc.TabStop = false;
            this.uc.UseVisualStyleBackColor = true;
            this.uc.Click += new System.EventHandler(this.om_Click);
            // 
            // ur
            // 
            this.ur.Location = new System.Drawing.Point(424, 54);
            this.ur.Name = "ur";
            this.ur.Size = new System.Drawing.Size(201, 146);
            this.ur.TabIndex = 2;
            this.ur.TabStop = false;
            this.ur.UseVisualStyleBackColor = true;
            this.ur.Click += new System.EventHandler(this.or_Click);
            // 
            // cl
            // 
            this.cl.Location = new System.Drawing.Point(29, 194);
            this.cl.Name = "cl";
            this.cl.Size = new System.Drawing.Size(201, 146);
            this.cl.TabIndex = 3;
            this.cl.TabStop = false;
            this.cl.UseVisualStyleBackColor = true;
            this.cl.Click += new System.EventHandler(this.ml_Click);
            // 
            // cc
            // 
            this.cc.Location = new System.Drawing.Point(226, 194);
            this.cc.Name = "cc";
            this.cc.Size = new System.Drawing.Size(201, 146);
            this.cc.TabIndex = 4;
            this.cc.TabStop = false;
            this.cc.UseVisualStyleBackColor = true;
            this.cc.Click += new System.EventHandler(this.mm_Click);
            // 
            // cr
            // 
            this.cr.Location = new System.Drawing.Point(424, 194);
            this.cr.Name = "cr";
            this.cr.Size = new System.Drawing.Size(201, 146);
            this.cr.TabIndex = 5;
            this.cr.TabStop = false;
            this.cr.UseVisualStyleBackColor = true;
            this.cr.Click += new System.EventHandler(this.mr_Click);
            // 
            // bl
            // 
            this.bl.Location = new System.Drawing.Point(29, 336);
            this.bl.Name = "bl";
            this.bl.Size = new System.Drawing.Size(201, 146);
            this.bl.TabIndex = 6;
            this.bl.TabStop = false;
            this.bl.UseVisualStyleBackColor = true;
            this.bl.Click += new System.EventHandler(this.ul_Click);
            // 
            // bc
            // 
            this.bc.Location = new System.Drawing.Point(226, 336);
            this.bc.Name = "bc";
            this.bc.Size = new System.Drawing.Size(201, 146);
            this.bc.TabIndex = 7;
            this.bc.TabStop = false;
            this.bc.UseVisualStyleBackColor = true;
            this.bc.Click += new System.EventHandler(this.um_Click);
            // 
            // turn
            // 
            this.turn.AutoSize = true;
            this.turn.Location = new System.Drawing.Point(46, 30);
            this.turn.Name = "turn";
            this.turn.Size = new System.Drawing.Size(37, 13);
            this.turn.TabIndex = 10;
            this.turn.Text = "----------";
            // 
            // groupBox1
            // 
            this.groupBox1.Controls.Add(this.turn);
            this.groupBox1.Location = new System.Drawing.Point(650, 54);
            this.groupBox1.Name = "groupBox1";
            this.groupBox1.Size = new System.Drawing.Size(139, 69);
            this.groupBox1.TabIndex = 11;
            this.groupBox1.TabStop = false;
            this.groupBox1.Text = "Turn";
            // 
            // br
            // 
            this.br.Location = new System.Drawing.Point(424, 336);
            this.br.Name = "br";
            this.br.Size = new System.Drawing.Size(201, 146);
            this.br.TabIndex = 8;
            this.br.TabStop = false;
            this.br.UseVisualStyleBackColor = true;
            this.br.Click += new System.EventHandler(this.ur_Click);
            // 
            // res
            // 
            this.res.Location = new System.Drawing.Point(650, 139);
            this.res.Name = "res";
            this.res.Size = new System.Drawing.Size(139, 23);
            this.res.TabIndex = 12;
            this.res.TabStop = false;
            this.res.Text = "Reset";
            this.res.UseVisualStyleBackColor = true;
            this.res.Click += new System.EventHandler(this.res_Click);
            // 
            // groupBox2
            // 
            this.groupBox2.Controls.Add(this.resetscore);
            this.groupBox2.Controls.Add(this.p2wins);
            this.groupBox2.Controls.Add(this.p1wins);
            this.groupBox2.Controls.Add(this.p2winstxt);
            this.groupBox2.Controls.Add(this.p1winstxt);
            this.groupBox2.Location = new System.Drawing.Point(650, 336);
            this.groupBox2.Name = "groupBox2";
            this.groupBox2.Size = new System.Drawing.Size(139, 146);
            this.groupBox2.TabIndex = 13;
            this.groupBox2.TabStop = false;
            this.groupBox2.Text = "Score";
            // 
            // resetscore
            // 
            this.resetscore.Location = new System.Drawing.Point(10, 93);
            this.resetscore.Name = "resetscore";
            this.resetscore.Size = new System.Drawing.Size(120, 23);
            this.resetscore.TabIndex = 4;
            this.resetscore.TabStop = false;
            this.resetscore.Text = "Reset";
            this.resetscore.UseVisualStyleBackColor = true;
            this.resetscore.Click += new System.EventHandler(this.resetscore_Click);
            // 
            // p2wins
            // 
            this.p2wins.AutoSize = true;
            this.p2wins.Location = new System.Drawing.Point(91, 37);
            this.p2wins.Name = "p2wins";
            this.p2wins.Size = new System.Drawing.Size(13, 13);
            this.p2wins.TabIndex = 3;
            this.p2wins.Text = "0";
            // 
            // p1wins
            // 
            this.p1wins.AutoSize = true;
            this.p1wins.Location = new System.Drawing.Point(10, 37);
            this.p1wins.Name = "p1wins";
            this.p1wins.Size = new System.Drawing.Size(13, 13);
            this.p1wins.TabIndex = 2;
            this.p1wins.Text = "0";
            // 
            // p2winstxt
            // 
            this.p2winstxt.AutoSize = true;
            this.p2winstxt.Location = new System.Drawing.Point(88, 20);
            this.p2winstxt.Name = "p2winstxt";
            this.p2winstxt.Size = new System.Drawing.Size(45, 13);
            this.p2winstxt.TabIndex = 1;
            this.p2winstxt.Text = "Player 2";
            // 
            // p1winstxt
            // 
            this.p1winstxt.AutoSize = true;
            this.p1winstxt.Location = new System.Drawing.Point(7, 20);
            this.p1winstxt.Name = "p1winstxt";
            this.p1winstxt.Size = new System.Drawing.Size(45, 13);
            this.p1winstxt.TabIndex = 0;
            this.p1winstxt.Text = "Player 1";
            // 
            // Form1
            // 
            this.AutoScaleDimensions = new System.Drawing.SizeF(6F, 13F);
            this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font;
            this.ClientSize = new System.Drawing.Size(813, 565);
            this.Controls.Add(this.groupBox2);
            this.Controls.Add(this.res);
            this.Controls.Add(this.br);
            this.Controls.Add(this.bc);
            this.Controls.Add(this.bl);
            this.Controls.Add(this.cr);
            this.Controls.Add(this.cc);
            this.Controls.Add(this.cl);
            this.Controls.Add(this.ur);
            this.Controls.Add(this.uc);
            this.Controls.Add(this.ul);
            this.Controls.Add(this.groupBox1);
            this.Name = "Form1";
            this.StartPosition = System.Windows.Forms.FormStartPosition.CenterScreen;
            this.Text = "TicTacToe";
            this.groupBox1.ResumeLayout(false);
            this.groupBox1.PerformLayout();
            this.groupBox2.ResumeLayout(false);
            this.groupBox2.PerformLayout();
            this.ResumeLayout(false);

        }

        #endregion

        private System.Windows.Forms.Button ul;
        private System.Windows.Forms.Button uc;
        private System.Windows.Forms.Button ur;
        private System.Windows.Forms.Button cl;
        private System.Windows.Forms.Button cc;
        private System.Windows.Forms.Button cr;
        private System.Windows.Forms.Button bl;
        private System.Windows.Forms.Button bc;
        private System.Windows.Forms.Button br;
        private System.Windows.Forms.Label turn;
        private System.Windows.Forms.GroupBox groupBox1;
        private System.Windows.Forms.Button res;
        private System.Windows.Forms.GroupBox groupBox2;
        private System.Windows.Forms.Label p2wins;
        private System.Windows.Forms.Label p1wins;
        private System.Windows.Forms.Label p2winstxt;
        private System.Windows.Forms.Label p1winstxt;
        private System.Windows.Forms.Button resetscore;
    }
}

This is the X i used This is the O i used

\$\endgroup\$
  • 1
    \$\begingroup\$ I'd be great if you could add the images to the question, \$\endgroup\$ – t3chb0t Oct 15 '18 at 9:34
  • 2
    \$\begingroup\$ If you could add the content of the file "Form1.Designer.cs", it'll be great too. \$\endgroup\$ – Henrik Hansen Oct 15 '18 at 10:11
4
\$\begingroup\$

First of all: as a first project this is not bad at all. It works, and the UI is usable and easy to understand.

That said there are a lot of things that could be done better/easier/smarter. I'm not able to comment it all but below find what I have:

The UI

The form is sizable, but the controls are static and do not resize with the form. That looks "unprofessional" and should be avoided. You can either set the FormBorderStyle on the form to one of the FixedXXX values or make the controls size/move along with the sizing of the form using the Anchor property of each control. It can be difficult with the button grid but you can experiment with a TableLayoutGrid as container.

From a users perspective it is a little annoying to be prompted for user names as the first - even before the main form is visible. Instead you could have input fields for the names in the main form directly, or you could rename the "Reset" button to "Start Game" and prompt for names when starting the first game.

In the "Turn" box it would be nice, if the signature (X or O) is shown adjacent to the player name, to make it obvious who is drawing.


The Code

In general you use a lot of abbreviated or short variable names. Do you self a favor and call the variables something meaningful and not abbreviated. It will make it easier to understand in five years when you need to revise the code.

The code has a lot of repetitions, which is considered as a bad design and it violates the DRY-principle.

For instance the click event handler for each button:

private void ol_Click(object sender, EventArgs e)
{
    if(player1 == true)
    {
        ul.Image = x;
        Playerstatus();
    }
    else
    {
        ul.Image = o;
        Playerstatus();
    }

    ul.Enabled = false;
}

Here you can use the same event handler for all the buttons, because the sender argument is a reference to the button that is clicked:

private void Button_Click(object sender, EventArgs e)
{
    Button button = sender as Button;
    button.Image = player1 ? x : o;
    PlayerStatus();
    button.Enabled = false;
}

In this way you can remove 8 almost identical eventhandlers.

In the same way you could combine P1win() and P2win() to one method:

private void EnableControls(bool enable)
{
  foreach (Control c in Controls) /* disables all buttons */
  {
    Button b = c as Button;
    if (b != null)
    {
      b.Enabled = enable;

    }
  }
}

public void SetWinner(string winner, ref int winnerPoints, Label label) /* Player 1 won */
{
  MessageBox.Show(winner + " won!");

  EnableControls(false);

  res.Enabled = true; /* reactivates the reset button */
  winnerPoints++;
  drawcount = 0;
  label.Text = winnerPoints.ToString();
  won = true;

}

The Checkwin() method is really ugly, and you should try something else. For instance create a matrix of buttons and then do some iterations:

private void SetWinner(Image image)
{
  if (image == x)
  {
    P1win();
  }
  else
  {
    P2win();
  }
}

public void Checkwin()
{
  Button[,] buttons =
  {
    { ul, uc, ur },
    { cl, cc, cr },
    { bl, bc, br }
  };

  for (int row = 0; row < 3; row++)
  {
    if (buttons[row, 0].Image != null && buttons[row, 0].Image == buttons[row, 1].Image && buttons[row, 1].Image == buttons[row, 2].Image)
    {
      SetWinner(buttons[row, 0].Image);
    }
  }

  // TODO: the same for columns and diagonals

  // Diagonals:
  if (buttons[0, 0].Image != null && buttons[0, 0].Image == buttons[1, 1].Image && buttons[1, 1].Image == buttons[2, 2].Image)
  {
    SetWinner(buttons[0, 0].Image);
  }
}

The above is just a soft review of the existing code. A more profound review results in a completely different "architecture" which comply to some common principles:

  1. Don't repeat yourself (DRY).
  2. Separation of concern.
  3. Model/View pattern

The code behind of the form should only respond to events from the UI (event handlers) and the state of the game model.

The game model is a class that holds the state and properties of the current game and could be something along these lines:

  public class TicTacToeGame
  {
    public TicTacToeGame(string xPlayerName, string oPlayerName)
    {
      CurrentPlayer = XPlayer = new Player(xPlayerName, Signature.X);
      OPlayer = new Player(oPlayerName, Signature.O);
    }

    public TicTacToeGame(Player xPlayer, Player oPlayer)
    {
      if (xPlayer.Signature != Signature.X || oPlayer.Signature != Signature.O) throw new ArgumentException("A player has wrong signature");

      CurrentPlayer = XPlayer = xPlayer;
      OPlayer = oPlayer;
    }

    private Signature[,] m_board = new Signature[3, 3];

    public Player XPlayer { get; }
    public Player OPlayer { get; }
    public Player CurrentPlayer { get; private set; }

    public bool IsGameOver()
    {
      Player winner = null;

      for (int row = 0; row < 3; row++)
      {
        if (m_board[row, 0] != Signature.None && m_board[row, 0] == m_board[row, 1] && m_board[row, 1] == m_board[row, 2])
        {
          winner = GetPlayer(m_board[row, 0]);
        }
      }

      for (int col = 0; col < 3; col++)
      {
        if (m_board[0, col] != Signature.None && m_board[0, col] == m_board[1, col] && m_board[1, col] == m_board[2, col])
        {
          winner = GetPlayer(m_board[0, col]);
        }
      }

      if (m_board[0, 0] != Signature.None && m_board[0, 0] == m_board[1, 1] && m_board[1, 1] == m_board[2, 2])
      {
        winner = GetPlayer(m_board[0, 0]);
      }

      if (m_board[2, 0] != Signature.None && m_board[2, 0] == m_board[1, 1] && m_board[1, 1] == m_board[0, 2])
      {
        winner = GetPlayer(m_board[2, 0]);
      }

      if (winner != null)
      {
        OnGameOver(winner);
      }

      return winner != null;
    }

    private Player GetPlayer(Signature signature)
    {
      switch (signature)
      {
        default:
        case Signature.None:
          return null;
        case Signature.O:
          return OPlayer;
        case Signature.X:
          return XPlayer;
      }
    }

    public void Draw(int row, int col)
    {
      if (row < 0 || row >= 3 || col < 0 || col >= 3) throw new ArgumentOutOfRangeException("row or col");
      if (m_board[row, col] != Signature.None) throw new InvalidOperationException("Field already set");

      m_board[row, col] = CurrentPlayer.Signature;

      OnDrawComitted(row, col);
      if (!IsGameOver())
      {
        CurrentPlayer = CurrentPlayer == XPlayer ? OPlayer : XPlayer;
      }
    }

    public event EventHandler<DrawEventArgs> DrawComitted;

    private void OnDrawComitted(int row, int col)
    {
      DrawComitted?.Invoke(this, new DrawEventArgs(row, col, CurrentPlayer));
    }

    public event EventHandler<GameOverEventArgs> GameOver;

    private void OnGameOver(Player winner)
    {
      GameOver?.Invoke(this, new GameOverEventArgs(winner));
    }
  }

  public enum Signature
  {
    None = -1,
    O = 0,
    X = 1,
  }

  public class Player
  {
    public string Name { get; }
    public Signature Signature { get; }
    public int WonGames { get; set; }

    public Player(string name, Signature type)
    {
      Name = name;
      Signature = type;
    }
  }

  public class DrawEventArgs : EventArgs
  {
    public DrawEventArgs(int row, int col, Player player)
    {
      Row = row;
      Column = col;
      Player = player;
    }

    public int Row { get; }
    public int Column { get; }
    public Player Player { get; }
  }


  public class GameOverEventArgs
  {
    public GameOverEventArgs(Player winner)
    {
      Winner = winner;
    }

    private Player Winner { get; }
  }
\$\endgroup\$
3
\$\begingroup\$

A Quick Note

First, welcome to CodeReview.SE! Secondly, welcome to the C# world! Your question in general is likely to receive a lot of opinion based answers, but from a CodeReview standpoint, I'll be glad to point out some things that could be updated. My review will exclude preference type changes such as brace placement; however, be warned that my code is formatted with the opening braces at end of line and excludes braces that aren't needed.


Expression Bodied Methods

So one cool thing I would like to start with is expression bodied methods. These things are awesome for methods you know will always be one line executions, and they can be applied to any method (including events)! Take your res_Click and tie methods for example:

private void res_Click(object sender, EventArgs e) {
    Resetgame();
}
public void tie() {
    MessageBox.Show("Tie!");
}

These can be easily converted into expression bodied methods:

private void res_Click(object sender, EventArgs e) => Resetgame();
public void tie() => MessageBox.Show("Tie!");

They will both still function in the same manner, but will allow you to clean up some of those excess braces (especially useful in large files with a lot of single line methods). A practical example of the usefulness of these are with implicit/explicit conversions and operand methods for custom data types. Say you have a two dimensional vector class, which can obviously be converted to three and four dimensional vector objects. There are a lot of methods that have to be written, and a lot of them are single lines:

public static implicit operator Vector4(Vector2 v) => return new Vector4(v.X, v.Y, 0, 0);
public static implicit operator Vector4(Vector3 v) => return new Vector4(v.X, v.Y, v.Z, 0);
public static operator Vector4+(Vector4 v1, Vector4 v2) => return new Vector4(v1.X + v2.X, v1.Y + v2.Y, v1.Z + v2.Z, v1.W + v2.W);

Documentation

Another thing I would like to point out is documentation for your code. Your method tie contains a comment /**/ on entry that tells us what it does. C# has a built in documentation feature that allows you to describe what a method, property, variable, argument, etc is for. For example:

/// <summary>
/// Displays a message box in the event of a tie.
/// </summary>
public void tie() => MessageBox.Show("Tie!");

The text window will actually generate this markup for you when you type /// on the line directly above a method, property, variable, etc. This is not only useful when someone else is reviewing your code, but also when you're putting out code for someone else to use in the form of a public API. This markup essentially what is being displayed to you when you're seeing intellisense!


Refactoring

There is basic level refactoring that can be done:

  • Logical Evaluations
  • Naming
  • if-elseif-else Structure

Overall, your code is well written and is easily readable and refactoring is definitely a matter of opinion. For example extra braces in the large logic tree in the Checkwin method; in my opinion, all of the extra braces aren't needed as they just take up space. I'll start with the easy one here being the logical evaluations in the Playerstatus method.

if (player1 == true)

Since player1 is a bool you can simply rewrite the if as:

if (player1)

I would change the name of the variable to something like player1sTurn, this makes it easier to understand, as I had to read the comment to ensure I knew what was going on there. I would also change the name of the method to something like DetermineWhosTurnItIs; it doesn't have to be as long, but a more descriptive name is better than a short name that doesn't make sense.

A good practice for naming conventions is:

  • Ensure the name is descriptive.
  • Use PascalCasing for variables.
  • Use camelCasing for methods and properties.
  • Use UPPER_SNAKE_CASE for constants.

I'm not saying that everyone follows this pattern, but a lot of companies still use these guidelines to this day. It's best to get into a good habit for naming conventions early on so that it's second nature in the professional world!

I would definitely add comments to your structure there to clarify what is happening in detail.

One of the biggest benefits I see for your code is utilizing a multi-dimensional array.


Using a Muli-Dimensional Array

Using a multi-dimensional array would be a big benefit in a game like Tic Tac Toe. For the simple reason that the game itself is a two-dimensional array. C# allows you to create multi-dimensional arrays and the easiest to follow is the two-dimensional array. Learning it now could help you later on with future projects such as Tetris! For example:

public char[,] gameBoard = new char[,] {
    { 'n', 'n', 'n' },
    { 'n', 'n', 'n' },
    { 'n', 'n', 'n' }
};

This allows you to visualize the board in code quite easily and is simple to modify!

// Change the top-right slot to an x.
gameBoard[0, 2] = 'x';
// Change the bottom middle slot to an o.
gameBoard[2, 1] = 'o';
// Change the middle first slot to an x.
gameBoard[1, 0] = 'x';

Then logical evaluation is also easy:

if (gameBoard[0, 0] == gameBoard[0, 1] && gameBoard[0, 1] == gameBoard[0, 2])
    if (gameBoard[0, 0] == 'x')
        P1win();
    else
        P2win();

Final Thoughts

I'm sure there are a few other things that could be touched up in your code, and there is usually 100 different ways to do the same thing when it comes to software development. Overall, your code was easy to understand and did what it was supposed to do, so +1 from me! My advice, just keep up the good work and keep learning! I wish you the best of luck in your future endeavors and I hope to see you come back to CodeReview.SE at some point in the future!

\$\endgroup\$
  • 1
    \$\begingroup\$ The suggested naming capitalization is wrong. The convention for C# is desribed here: docs.microsoft.com/en-us/dotnet/standard/design-guidelines/… Basically, camelCase for parameters and local variables, PascalCase for evertything else, including constants. \$\endgroup\$ – ya23 Oct 17 '18 at 13:01
  • 1
    \$\begingroup\$ @ya23 I wouldn't say it is wrong, but it's a very good point to bring up Microsoft's conventions! I'll update my post later; I just spoke from my experience in the professional world. \$\endgroup\$ – PerpetualJ Oct 17 '18 at 14:11
  • 1
    \$\begingroup\$ Thanks, that would be great! By wrong I meant it's not an idiomatic style. Every language tends to follow certain guidelines, not following them makes it look odd - for example look at the method naming in .NET class library - they are all Pascal case. Camel case methods and opening curly brace on the same line as method name is perfectly fine in Java code, but looks weird in C#. Ifs without curly braces is a more controversial point, but when nested, I'd definitely add a brace around them. Without it, the code looks a bit like Python ;) \$\endgroup\$ – ya23 Oct 18 '18 at 9:41

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.