Tic Tac Toe in the Console

Question

I have made a Tic Tac Toe game in C#. How can I improve my code? The full listing is as follows:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Threading;

namespace TicTacToe1
{
    class Program
    {
        private char winChar;
        public char winPerson
        {
            get
            {
                return winChar;
            }
            set
            {
                winChar = value;
            }
        }
        private bool hasWon;
        public bool isWin
        {
            get
            {
                return hasWon;
            }
            set
            {
                hasWon = value;
            }
        }
        private bool isX;
        public bool isY
        {
            get
            {
                return isX;
            }
            set
            {
                isX = value;
            }
        }
        private char boxone, boxtwo, boxthree, boxfour, boxfive, boxsix, boxseven, boxeight, boxnine;
        public char box1
        {
            get
            {
                return boxone;
            }
            set
            {
                boxone = value;
            }
        }
        public char box2
        {
            get
            {
                return boxtwo;
            }
            set
            {
                boxtwo = value;
            }
        }
        public char box3
        {
            get
            {
                return boxthree;
            }
            set
            {
                boxthree = value;
            }
        }
        public char box4
        {
            get
            {
                return boxfour;
            }
            set
            {
                boxfour = value;
            }
        }
        public char box5
        {
            get
            {
                return boxfive;
            }
            set
            {
                boxfive = value;
            }
        }
        public char box6
        {
            get
            {
                return boxsix;
            }
            set
            {
                boxsix = value;
            }
        }
        public char box7
        {
            get
            {
                return boxseven;
            }
            set
            {
                boxseven = value;
            }
        }
        public char box8
        {
            get
            {
                return boxeight;
            }
            set
            {
                boxeight = value;
            }
        }
        public char box9
        {
            get
            {
                return boxnine;
            }
            set
            {
                boxnine = value;
            }
        }
        public void WriteBoard()
        {
            Console.WriteLine(" {0} | {1} | {2} ", boxone, boxtwo, boxthree);
            Console.WriteLine(" --------- ");
            Console.WriteLine(" {0} | {1} | {2} ", boxfour, boxfive, boxsix);
            Console.WriteLine(" --------- ");
            Console.WriteLine(" {0} | {1} | {2} ", boxseven, boxeight, boxnine);
        }
        public void CheckWin()
        { // 123, 456, 789, 147, 258, 369, 159, 357
            if ((box1 == 'X') && (box2 == 'X') && (box3 == 'X'))
            {
                isWin = true;
                winPerson = 'X';
                return;
            }
            if ((box4 == 'X') && (box5 == 'X') && (box6 == 'X'))
            {
                isWin = true;
                winPerson = 'X';
                return;
            }
            if ((box7 == 'X') && (box8 == 'X') && (box9 == 'X'))
            {
                isWin = true;
                winPerson = 'X';
                return;
            }
            if ((box1 == 'X') && (box4 == 'X') && (box7 == 'X'))
            {
                isWin = true;
                winPerson = 'X';
                return;
            }
            if ((box2 == 'X') && (box5 == 'X') && (box8 == 'X'))
            {
                isWin = true;
                winPerson = 'X';
                return;
            }
            if ((box3 == 'X') && (box6 == 'X') && (box9 == 'X'))
            {
                isWin = true;
                winPerson = 'X';
                return;
            } // 159, 357
            if ((box1 == 'X') && (box5 == 'X') && (box9 == 'X'))
            {
                isWin = true;
                winPerson = 'X';
                return;
            }
            if ((box3 == 'X') && (box5 == 'X') && (box7 == 'X'))
            {
                isWin = true;
                winPerson = 'X';
                return;
            }
            if ((box1 == 'Y') && (box2 == 'Y') && (box3 == 'Y'))
            {
                isWin = true;
                winPerson = 'Y';
                return;
            }
            if ((box4 == 'Y') && (box5 == 'Y') && (box6 == 'Y'))
            {
                isWin = true;
                winPerson = 'Y';
                return;
            }
            if ((box7 == 'Y') && (box8 == 'Y') && (box9 == 'Y'))
            {
                isWin = true;
                winPerson = 'Y';
                return;
            }
            if ((box1 == 'Y') && (box4 == 'Y') && (box7 == 'Y'))
            {
                isWin = true;
                winPerson = 'Y';
                return;
            }
            if ((box2 == 'Y') && (box5 == 'Y') && (box8 == 'Y'))
            {
                isWin = true;
                winPerson = 'Y';
                return;
            }
            if ((box3 == 'Y') && (box6 == 'Y') && (box9 == 'Y'))
            {
                isWin = true;
                winPerson = 'Y';
                return;
            } // 159, 357
            if ((box1 == 'Y') && (box5 == 'Y') && (box9 == 'Y'))
            {
                isWin = true;
                winPerson = 'Y';
                return;
            }
            if ((box3 == 'Y') && (box5 == 'Y') && (box7 == 'Y'))
            {
                isWin = true;
                winPerson = 'Y';
                return;
            }
        }
        public void NotVacantError()
        {
            _error = true;
            Console.WriteLine();
            Console.WriteLine("Error: box not vacant!");
            Console.WriteLine("Press any key to try again..");
            Console.ReadKey();
            return;
        }
        public void DisplayLoss()
        {
            Console.WriteLine();
            Console.WriteLine("No one won.");
            Console.ReadKey();
            Environment.Exit(1);
        }
        private bool hasError;
        public bool _error
        {
            get
            {
                return hasError;
            }
            set
            {
                hasError = value;
            }
        }
        static void Main()
        {
            int moveCount = 0; // check loss
            char askMove; // display X or Y in question
            int selTemp;
            TicTacToe1.Program prog = new TicTacToe1.Program();
            prog._error = false;
            prog.box1 = ' '; prog.box2 = ' '; prog.box3 = ' ';
            prog.box4 = ' '; prog.box5 = ' '; prog.box6 = ' ';
            prog.box7 = ' '; prog.box8 = ' '; prog.box9 = ' ';
            prog.isY = true;
            Console.WriteLine(" -- Tic Tac Toe -- ");
            Thread.Sleep(1200);
            Console.Clear();
            while (!prog.isWin)
            {
                if (moveCount == 9)
                {
                    prog.DisplayLoss();
                }
                if ((prog.isY) == true) // if is X
                {
                    askMove = 'X';
                }
                else
                {
                    askMove = 'Y';
                }
                Console.Clear();
                prog.WriteBoard();
                Console.WriteLine();
                Console.WriteLine("What box do you want to place {0} in? (1-9)", askMove);
                Console.Write("> ");
                selTemp = int.Parse(Console.ReadLine());
                switch (selTemp)
                {
                    case 1:
                        if (prog.box1 == ' ')
                        {
                            prog.box1 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    case 2:
                        if (prog.box2 == ' ')
                        {
                            prog.box2 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    case 3:
                        if (prog.box3 == ' ')
                        {
                            prog.box3 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    case 4:
                        if (prog.box4 == ' ')
                        {
                            prog.box4 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    case 5:
                        if (prog.box5 == ' ')
                        {
                            prog.box5 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    case 6:
                        if (prog.box6 == ' ')
                        {
                            prog.box6 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    case 7:
                        if (prog.box7 == ' ')
                        {
                            prog.box7 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    case 8:
                        if (prog.box8 == ' ')
                        {
                            prog.box8 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    case 9:
                        if (prog.box9 == ' ')
                        {
                            prog.box9 = askMove;
                            moveCount++;
                        }
                        else
                        {
                            prog.NotVacantError();
                        }
                        break;
                    default:
                        Console.WriteLine("Wrong selection entered!");
                        Console.WriteLine("Press any key to try again..");
                        Console.ReadKey();
                        prog._error = true;
                        break;
                }
                if (prog._error)
                {
                    prog.CheckWin(); // if error, just check win
                    prog._error = !prog._error;
                }
                else
                {
                    prog.isY = !prog.isY; // flip boolean
                    prog.CheckWin();
                }
            }
            Console.Clear();
            prog.WriteBoard();
            Console.WriteLine();
            Console.WriteLine("The winner is {0}!", prog.winPerson);
            Console.ReadKey();
        }
    }
}

Malachi · Accepted Answer · 2014-12-10 21:50:25Z

This doesn't make sense to me

    private bool isX;
    public bool isY
    {
        get
        {
            return isX;
        }
        set
        {
            isX = value;
        }
    }

Are you checking for X or Y I assume you want 2 separate methods here or you just named these incorrectly.

Here is where that incorrectly named variable comes back to bite you

if ((prog.isY) == true) // if is X
{
    askMove = 'X';
}
else
{
    askMove = 'Y';
}

You had to write a comment here to explain what was going on, when it should be straight forward.

my suggestion is to change

private bool isX;

to

private bool _isX;

and then rewrite your public call like this

public bool isX
{
    get
    {
         return _isX;
    }
    set
    {
         _isX = value;
    }
}

or better yet, get rid of the private declaration and write the whole thing like this

public bool isX
{
    get;
    set;
}

your main method has a lot going on inside of it that you should probably give to other methods and just call them in the main method.

like

            if (moveCount == 9)
            {
                prog.DisplayLoss();
            }

could be it's own method called CheckForStaleMate()

and your switch (selTemp) could be inside a Method called CheckMove()

You could also create a method for your output as well. There shouldn't be much in your main method, it should be clean and should only call methods or possibly create an object or two.

BCdotWEB · Accepted Answer · 2014-12-10 15:48:15Z

Some quick remarks:

public properties should be PascalCase.
use Auto-Implemented Properties.
Capitalize the various parts of boxone etc., e.g. boxOne. Avoid using numbers in your names, e.g. box9.
Do not start properties with an underscore: public bool _error.
CheckWin() repeats the same code over and over again, surely it can be reduced to something far simpler by calling another method with some parameters.
Same for switch (selTemp) in your Main(): surely that can be significantly reduced.
If you have properties, why do you use the private fields and not the properties?
Don't do this: private char boxone, boxtwo, boxthree, boxfour, boxfive, boxsix, boxseven, boxeight, boxnine;
Is there even a need for the individual declaration of boxes? Wouldn't it be simpler to store the values of the nine positions in an array or a dictionary?

user2023861 · Accepted Answer · 2014-12-10 17:24:04Z

Why not store moves as a string of 0s and 1s instead of 9 properties? For instance this:

_ X _
_ X O
X O _

could be represented as:

string xmoves = "010010100"
string omoves = "000001010"

With this scheme, you can store all of the winning moves (there are only 8) in an array of strings:

string[] winningMoves = {
    "111000000",
    "000111000", ... }

Then to check if x wins, you have a simple algorithm like this:

static bool IsWin(string moves, string[] winningMoves)
{
    foreach (var item in winningMoves)
    {
        int count = 0;
        for (int i = 0; i < item.Length; i++)
        {
            if (item[i] == '1' && moves[i] == '1')
            {
                count++;
                if (count == 3)
                    return true;
            }
        }
    }
    return false;
}

You'll need a function that checks if a move is possible. This is easy:

static bool CanMove(int index, string xmoves, string ymoves)
{
    return xmoves[index] == '0' && ymoves[index] == '0';
}

Finally, to make a move, you need to do a little string concatenation:

static string Move(int index, string moves)
{
    return string.Format("{0}1{1}", moves.Substring(0, index), moves.Substring(index + 1));
}

I'd be tempted to store the moves in binary rather than a string so that I can utilize bit-level operations. This would make the code uglier however. For instance, CanMove would look like:

static bool CanMove(int index, int xmoves, int ymoves)
{
    int bit = 1 << index;
    return (xmoves & bit) == 0 && (ymoves & bit) == 0;
}

Which I find much less intuitive.

I honestly don't think this is a good way of structuring the moves. There is some constraints that needs to be applied on the strings. For example, they should only contain 0's and 1's - number 2 in a string would have no meaning. Additionally, a '1' must not occur in the same index in both strings. These things are telling me that a better structure would be a 2d array of an enum. — Simon Forsberg, Dec 10, 2014 at 20:04
@SimonAndréForsberg, I agree that the programmer should be aware of these problems. Other problems involve the fact that I assume strings are nine chars long, or that I'm making an assumption about the indexing of the tic-tac-toe board. If the programmer properly encapsulates my code, these problems won't be an issue. I wanted to illustrate a more succinct solution to the OP. I might change the the CanMove() and Move() methods to one TryMove() method though. — user2023861, Dec 10, 2014 at 20:28

Horius · Accepted Answer · 2014-12-16 14:48:36Z

5

You should definitely think about renaming your identifiers. It is important that you are able to recognize the code on your own a few weeks later, so you should maybe look at things like the Hungarian notation, which are about prefixing the identifiers. The identifiers should also have definite names which are recognizable for you later on.

One possibility to avoid code like this

private bool isX;
public bool isY
{
    get
    {
        return isX;
    }
    set
    {
        isX = value;
    }

is to call isX fbIsX and calling the Property implementation of it isX.

edited Dec 16, 2014 at 14:48

answered Dec 10, 2014 at 15:43

Horius

673 bronze badges

2

\$\begingroup\$ Hi, welcome to Code Review! Your first answer looks good, you could also always improve answers by linking to relevant concepts, or further details (e.g. to Wikipedia on Hungarian notation). \$\endgroup\$
– ferada
Dec 10, 2014 at 15:55
\$\begingroup\$ Thanks for the hint, i will keep it in mind for my next answer :) \$\endgroup\$
– Horius
Dec 10, 2014 at 15:57
\$\begingroup\$ Hungarian Notation isn't preferable to other naming schemes like the underscore before the private property name, ie (private _isX -> public isX). what is fb supposed to stand for anyway? \$\endgroup\$
– Malachi ♦
Dec 10, 2014 at 16:08
2

\$\begingroup\$ Probably worth noting that hungarian notation is very non-standard in modern C# (except I guess for the conventional I at the beginning of interface names and T at the beginning of generic type parameter names) \$\endgroup\$
– Ben Aaronson
Dec 10, 2014 at 18:14
1

\$\begingroup\$ Just chiming in here... Hungarian Notation itself isn't evil. It's just hard to do it right. And @Malachi you're thinking of Apps hungarian, which is agreed by all to be a horrible idea. \$\endgroup\$
– RubberDuck
Dec 16, 2014 at 15:08

| Show 4 more comments

user622505 · Accepted Answer · 2014-12-11 13:07:04Z

You don't do anything non-standard in your getters or setters. Instead of:

private char winChar;
public char winPerson
{
    get
    {
        return winChar;
    }
    set
    {
        winChar = value;
    }
}

Just do:

public char WinPerson { get; set; }

Much more readable and does exactly the same.

Public property names are by convention supposed to be PascalCase, so I called it WinPerson instead of winPerson.

Also, if for some reason you DO need those private fields declared explicitly, don't name them differently - just make them start with lower case or _. Like this:

private char _winPerson;
public char WinPerson
{
    get
    {
        // do some more things, so this kind of property is justified
        return _winPerson;
    }
    set
    {
        _winPerson= value;
    }
}

Stack Exchange Network

Tic Tac Toe in the Console

5 Answers 5

Your Answer

Not the answer you're looking for? Browse other questions tagged
c#
game
console
tic-tac-toe
or ask your own question.

Hot Network Questions

Tic Tac Toe in the Console

5 Answers 5

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c#gameconsoletic-tac-toe or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
c#
game
console
tic-tac-toe
or ask your own question.