Tic Tac Toe using WinForms

Question

I have made a simple tic tac toe game and need help tweaking it to fit the bill of object oriented design. Any suggestions most welcome. I basically want it safer, and more "best practice-like" It is a game consisting of four classes, and I'm pretty happy with it as I am new to programming, but maybe it is worthless seen from your perspective. Any pointers, tips and concrete examples would be greatly appreciated!

Board class:

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

namespace TicTac
{
    class Board
    {
        private int movesMade = 0; //Keeps tracks of how many moves made

        private Holder[,] holders = new Holder[3, 3]; //Creates an array of 3x3 

        private const int X = 0; //Whatever X presses becomes a 0 in the array
        private const int O = 1; //Whatever O presses becomes a 1 in the array
        public const int B = 2; //Whatever space is unclicked is a 2 in the array

        public void initBoard() //Iniates the board
        {
            for (int x = 0; x < 3; x++) //Vertical row
            {
                for (int y = 0; y < 3; y++) //Horizontal row
                {
                    holders[x, y] = new Holder();
                    holders[x, y].setValue(B);
                    holders[x, y].setLocation(new Point(x, y));
                }
            }
        }
        public void detectHit(Point loc) //Checks what space was hit and draws either an O or X depending on whos turn it is
        {
            if (loc.Y <= 500)
            {
                int x = 0;
                int y = 0;

                if (loc.X < 167)
                {
                    x = 0;
                }
                else if (loc.X > 167 && loc.X < 334)
                {
                    x = 1;
                }
                else if (loc.X > 334)
                {
                    x = 2;
                }
                if (loc.Y < 167)
                {
                    y = 0;
                }
                else if (loc.Y > 167 && loc.Y < 334)
                {
                    y = 1;
                }
                else if (loc.Y > 334 && loc.Y < 500)
                {
                    y = 2;
                }

                movesMade++;

                if (movesMade % 2 == 0) //If even number X turns to act
                {

                    GFX.putX(new Point(x, y));
                    holders[x, y].setValue(X);
                    if (checkWinner(X))
                    {
                        MessageBox.Show("Player 'X' won");
                    }

                 }


                     else
                     {

                    GFX.putO(new Point(x, y)); //Calls "Put O" method when its his turn
                    holders[x, y].setValue(O);
                    if (checkWinner(O))
                    {
                        MessageBox.Show("Player 'O' Won!");                        
                    }

                }
                // MessageBox.Show(x.ToString() + "," + y.ToString());
            }

        }

        private bool checkWinner(int Z)
        {
            bool winner = false;

            for (int x = 0; x < 3; x++)
            {
                if (holders[x, 0].getValue() == Z && holders[x, 1].getValue() == Z && holders[x, 2].getValue() == Z) //Checks if board is won horizontally
                {
                    return true;
                }
                if (holders[0, x].getValue() == Z && holders[1, x].getValue() == Z && holders[2, x].getValue() == Z) //Checks if board is won vertically
                {
                    return true;
                }
                switch (x)          //Checks if board is won diagnonally
                {
                    case 0:
                        if (holders[0, 0].getValue() == Z && holders[1, 1].getValue() == Z && holders[2, 2].getValue() == Z) 
                        {
                            return true;
                        }
                        break;

                    case 1:
                        if (holders[0, 2].getValue() == Z && holders[1, 1].getValue() == Z && holders[0, 2].getValue() == Z)
                        {
                            return true;
                        }
                        break;                   
                }
            }

            return winner;
        }
    }
}

Holder class:

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

namespace TicTac
{
   public class Holder
    {
        private Point loc;
        public int value = Board.B;

        internal void setLocation(Point p)
        {
            loc = p;
        }
        internal Point getLocation()
        {
            return loc;
        }

        internal void setValue(int i)
        {
            value = i;
        }

        internal int getValue()
        {
            return value;
        }
    }
}

Graphics class:

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

namespace TicTac
{
    class GFX
    {
        private static Graphics gObject;
        public GFX(Graphics g)
        {
            gObject = g;
            initBoard();
        }

        public void initBoard() //Method that initiates the boaard
        {
            Pen lines = new Pen(Color.Chocolate, 6);
            Brush bg = new SolidBrush(Color.White);

            gObject.FillRectangle(bg, new Rectangle(0, 0, 500, 600)); //Draws the background

            gObject.DrawLine(lines, new Point(167, 0), new Point(167, 500)); //Draws the lines
            gObject.DrawLine(lines, new Point(334, 0), new Point(334, 500)); //Draws the lines

            gObject.DrawLine(lines, new Point(0, 167), new Point(500, 167)); //Draws the lines
            gObject.DrawLine(lines, new Point(0, 334), new Point(500, 334)); //Draws the lines

        }

        public static void putX(Point loc) //Method that writes out an X
        {
            Pen xObjekt = new Pen(Color.Blue, 4);
            int xAbs = loc.X * 167;
            int yAbs = loc.Y * 167;

            gObject.DrawLine(xObjekt, xAbs + 20, yAbs + 20, xAbs + 157, yAbs + 157); //Makes an X when board is clicked, adjusted to not intervene with next square
            gObject.DrawLine(xObjekt, xAbs + 157, yAbs + 20, xAbs + 20, yAbs + 157); //Makes an X when board is clicked, adjusted to not intervene with next square


        }
        public static void putO(Point loc) //Method that writes out an O
        {
            Pen oObjekt = new Pen(Color.Green, 4);

            int xAbs = loc.X * 167;
            int yAbs = loc.Y * 167;

            gObject.DrawEllipse(oObjekt, xAbs + 20, yAbs + 20, 120, 120);          //Makes an O when board is clicked, adjusted to not intervene with next square


        }
    }

}

UI_FORM1:

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

namespace TicTac
{
    public partial class Form1 : Form
    {
         Board board;
         GFX engine;

        public Form1()
        {
            InitializeComponent();
        }

        private void panel1_Paint(object sender, PaintEventArgs e)
        {

        }

        private void panel1_Click(object sender, EventArgs e) //When player interacts with the board
        {
            Point mouse = Cursor.Position;
            mouse = panel1.PointToClient(mouse);
            board.detectHit(mouse);
        }

        private void gameToolStripMenuItem_Click(object sender, EventArgs e) //When a singleplayermode is selected
        {
            Graphics toPass = panel1.CreateGraphics();
            engine = new GFX(toPass);

            board = new Board();
            board.initBoard();
        }

    }
}

Answer 1

Method names should be UpperCamelCase. In C#, the standard way to allow public access to variables is using properties. Your Holder class would look like this:

public class Holder
{
    private Point loc;
    // "value" is actually a special name used for setting properties,
    // so it would be confusing to give a variable the same name.
    private int val = Board.B;

    internal Point Location
    {
        get { return loc; }
        set { loc = value; }
    }

    internal int Value
    {
        get { return val; }
        set { val = value; }
    }
}

You can remove the need for private variables by using Auto-implemented properties:

public class Holder
{
    internal Point Location { get; set; }
    // Initializing an auto-implemented property requires using C# 6 or later.
    internal int Value { get; set; } = Board.B;
}

There are magic numbers everywhere. If you decide to change the size of your board, you are going to have to change a lot of numbers. Normally you can just declare them as constants privately in a class, but because they are used by multiple classes I recommend extracting the magic numbers into a Constants class.

You shouldn't have public fields. The following:

public const int B = 2;

Should be written like this:

public int B { get; } = 2;

If you have a group of related constants, and it doesn't matter what their values are, you can put them in an enum. So the values could look like this:

public enum Values { X, O, B }

class Board
{
// ...

Then checkWinner() can have a method signature like this:

private bool CheckWinner(Values value)

The detectHit() method can be improved. It doesn't check if loc.X or loc.Y are equal to 167 or 334, so, for example, if loc.X and loc.Y were both 334, then x and y would both be 0. I recommend checking for all values you don't want in the first if statement. So it could look like this (remember to use constants):

if (loc.Y < 500 && loc.Y != 167 && loc.Y != 334 && loc.X < 500 && loc.X != 167 && loc.X != 334)
{

Now that the unwanted values are taken care of, you can assign x and y using loops:

int x = 0;
int y = 0;

int[] edges = new int[] { 167, 334, 500 };
for (int i = 0; i < edges.Count(); i++)
{
    if(loc.X < edges[i])
    {
        x = i;
        break;
    }
}
for (int i = 0; i < edges.Count(); i++)
{
    if(loc.Y < edges[i])
    {
        y = i;
        break;
    }
}

Since the for loops are similar, you could extract one into a method and then replace them with two method calls. I'm going to replace the loops using LINQ. This requires using System.Linq;

int[] edges = new int[] { 167, 334, 500 };
int x = Enumerable.Range(0, edges.Count()).First(i => loc.X < edges[i]);
int y = Enumerable.Range(0, edges.Count()).First(i => loc.Y < edges[i]);

In checkWinner(), you can move the switch statement outside of the loop and get rid of the switch part, keeping the if statements. You can also combine the predicates of the successive if statements using ||.

If you want, you can calculate the "constants" using a specified board width a edge width.