Command Prompt TicTacToe in C#

Question

I wrote a basic command prompt Tic Tac Toe Game. I want to know what can be improved in terms of modeling and what mistakes I've made (if any).

void Main()
{
    TicTacToeBoard board = new TicTacToeBoard();

    while(!board.GameOver())
    {
        var i = int.Parse(Console.ReadLine());
        board.MakeMove(i);
        Console.WriteLine(board.DrawBoard());
    }   
}

class TicTacToeBoard
{
    SquareType[] board = new SquareType[9];

    SquareType currentType = SquareType.X;

    public void MakeMove(int i)
    {
        if(board[i] != SquareType.Empty) return;
            board[i] = currentType;
            currentType = OppositeType(currentType);
    }

    SquareType OppositeType(SquareType squareType) => 
        squareType == SquareType.X ? SquareType.O : SquareType.X;

    public bool GameOver()
    {
        return WeHaveWinner() || WeHaveDraw();
    }

    bool WeHaveWinner()
    {
        var winningIndecies = new[]
        {
            new[]{1,2,3},
            new[]{4,5,6},
            new[]{7,8,9},
            new[]{1,5,9},
            new[]{3,5,7}
        };

        foreach(var indecies in winningIndecies)
        {
            var squares = indecies.Select(i => board[i - 1]);
            if(squares.Any(x => x == SquareType.Empty)) continue;
            else if(squares.Distinct().Count() == 1) return true;
        }
        return false;
    }

    bool WeHaveDraw() => !WeHaveWinner() && board.All(x => x != SquareType.Empty);


    public string DrawBoard()
        => $@"|{board[0]},{board[1]},{board[2]}|
|{board[3]},{board[4]},{board[5]}|
|{board[6]},{board[7]},{board[8]}|".Replace("Empty","  ");
}

enum SquareType { Empty, X, O }

Answer 1

Overall it looks good to me. A few points though:

1. int.Parse will throw if Console.ReadLine returns a non integer. Perhaps an int.TryParse and display some help (position: 0 - 8)
2. It's a small app but since for the whole lifetime of your application, in each iteration you'll call WeHaveWinner. I'd initialize that array as a field to that class on object construction.
3. On WeHaveWinner you'll (possibly) be enumerating the return of .Select() twice. Consider calling ToList() on it.
4. The main method, could be:

      do
        {
            var i = int.Parse(Console.ReadLine());
            board.MakeMove(i);
            Console.WriteLine(board.DrawBoard());
        } while (!board.GameOver());

This way you won't be checking for winnings before the first input

5. Your string replace at the end on Empty could lead to a issue if the Enum Empty was renamed. Consider replacing (SquareType.Empty, " ")
6. You could consider replacing the Enum with a class, that would give you ability to use polymorphism. There you could override ToString to display (X or Y or empty) which would help you get rid of that String.Replace altogether. Something along the lines of: https://stackoverflow.com/questions/1376312/whats-the-equivalent-of-javas-enum-in-c

+1 for the verbatin + interpolation

Answer 2

Overall, I think you've done a good job, with excellent logic, etc. However there are some issues, mostly OOP and language (C#) specific. In particular, the code has a heavy procedural style to it. Additionally, these methods look like something a Java programmer would do - C# has first class properties, so I'm not even sure why some of them are methods.

---

Naming Conventions

I am noticing the use of 'we' in your method names (WeHaveWinner and WeHaveDraw). Your TicTacToeBoard is not a we, it is only one thing. TicTacToeBoard.HaveWinner() or TicTacToeBoard.HasWinner() would be much better than the current TicTacToeBoard.WeHaveWinner(). The same goes for WeHaveDraw().

Now for a rant:

As a general rule of thumb, you should think of an instance reference (i.e. the local variable board in Main()), as representing A (single) TicTacToeBoard in reality (except that in this reality you can talk to it, via message passing a.k.a. methods). Then each method you invoke against it, can be thought of as asking it things.. However, these things are about that object, not it's environment. You shouldn't ask a TicTacToe Board "do you we have a winner?". If you think of your method names as having to come after the words 'do you', or 'what is your' you should usually get much better names.

---

Objects should have properties

Next up, since you've changed the names, perhaps it's time to change their structure - these methods read a lot more like properties to me. In some OOP languages like Java (and others), properties are implemented as methods (technically 'accessor methods' in Java, a.k.a. getters), but if you're using a very nice OOP language like C#, which has first-class properties, then you may as well make use of them!

As an example, instead of:

public bool GameOver()
{
    return WeHaveWinner() || WeHaveDraw();
}

which then requires invocation (i.e. the brackets ()) when called: GameOver(), you should instead make this into a property:

public bool IsGameOver
{
    get
    {
        return WeHaveWinner() || WeHaveDraw();
    }
}

This will mean you can retrieve the value of TicTacToeBoard.GameOver() by using TicTacToeBoard.IsGameOver. You can also apply this to the HaveDraw/HasDraw and HaveWinner/HasWinner methods I mentioned earlier. (And then obviously change the implementation of IsGameOver to use the property syntax to access them too.)

---

Some type of variable?

Not that this is in any way invalid or wrong, but the use of var (variable type) seems somewhat high in this code. This often happens out of habit for people coming from JavaScript or VB.NET (which uses Dim). In C# you can just specify the type (ala C/C++ style) before the variable name.

However, there is nothing wrong with a line like:

var i = int.Parse(Console.ReadLine());

But, it is the exact same size as:

int i = int.Parse(Console.ReadLine());

and this is more readable for others, as they will look at the beginning of the declaration for the type. The same things goes for the winningIndecies variable and your foreach loop iterating variable in WeHaveWinner - neither of which have particularly complicated type signatures.