Follow-Up Post Tictactoe now with AI

A month ago I finished and posted my first version of my Tic-Tac-Toe, I received a lot of good feedback and refactored it to more closely suit the MVP design pattern.

I've done some further clean-ups and refactoring in the code now but most importantly I have implemented a very primitive AI, that works "fairly well", my goal with the AI was NOT to create an AI with perfect play, this has been covered many times before as Tictactoe is a solved game, my goal was to create an AI that would mimick human play more closely. I think the implementation is decent but the code is a mess, it was my first attempt ever at coding a weak AI and it ended up with me having to add quite a large amount of extra methods for searching and sorting the grid just for the AI to work.

Anyhow, any feedback is appreciated, not restricted to the AI only, I have posted the (almost) complete code below:

Grid.cs

```
class Grid
{
const int MAX_CELLS = 3;
public Cell[,] Cells { get; set; }
private Cell[,] cells = new Cell[MAX_CELLS, MAX_CELLS];
public Cell this[int index, int index2]
{
get
{
return cells[index, index2];
}
set
{
cells[index, index2] = value;
}
}
public Outcome Outcome { get; private set; }

public Grid(IGameViewer viewer)
{
Outcome = Outcome.None;

for (int x = 0; x e.Equals(coords)) || middle.Equals(coords)) { checkDiagonals = true; }

if (player.PlayerWon(this[coords.X, coords.Y], this, checkDiagonals))
{
switch (player.marker)
{
case Mark.Cross:
Outcome = Outcome.CrossWin;
break;
case Mark.Nought:
Outcome = Outcome.NoughtWin;
break;
}

Fields

I see a property Cells but it is never used either in Grid or any other class (as far as I can tell).

Order your class members like this:

• Constants

• Fields

• Properties

• Methods

This makes it easier for people to read the code and have an expectation of where they can find what.

Naming

index and index2 aren't very descriptive names. Consider naming them according to their purpose: row and column.

Your method CheckOutcome returns a bool which is cleared up in the comments inside the method that true means someone won and false nobody won. The current method name would suggest a void return type, perhaps returning a control-variable (like -1 or true/false for errors) or an exception. If you change it to, for example, HasWinner then the intent is immediately more clear.

You have a property called DisallowPlay, which is the inversion of AllowPlay. People find it easier to read the positive version and things like AllowPlay = true make more sense than DisallowedPlay = false.

I would change DiagonalWin and DiagonalWin2 to DiagonalWinLeftTop and DiagonalWinRightTop (or similar values). That way you know what diagonal it is exactly.

Magic values

Magic values are values that look like they're placed there randomly. Consider adding context to make this immediately clear.

For example:

var corners = new Position[] { new Position(0,0), new Position(2,0), new Position(0,2), new Position(2,2) };

could become

int boundary = MAX_CELLS - 1;
var corners = new Position[] { new Position(0,0), new Position(boundary ,0), new Position(0, boundary ), new Position(boundary , boundary) };

Added benefit: now it's also easier to provide support for bigger playfields, you just have to manipulate the MAX_CELLS constant.

Consistency

I prefer == over .Equals(). It gives me less the feeling as if the code is all cluttered which makes me feel all happy and fluffy. You use == everywhere as well, so unless you overloaded the operator (which I can't tell), I would change

cells[x, y].Equals(cell)

to

cells[x, y] == cell

404 values

-1 is a common way to express "not found" when you're expecting an integer but using the same approach to fill a referency type's fields is not the intention. How is the programmer supposed to know if he should check position.x? Or position.y? Maybe both?

In rare cases, returning null is an appropriate solution. I would say that this is one of them (method: public Position IndexOf(Cell cell)).

I notice there is never any check done to see if the cell is invalid so I assume you just work with them and let the program flow iterate over it like a valid position. I don't know how the code works in depth but keep in mind that this means some properties might not get initialized because they get skipped somewhere which in turn can cause a NullReferenceException elsewhere where they get read.

Initialization

This is not needed:

Turn = new int();

Threading

I'm not very experienced with multithreading but unless I'm mistaking, you're putting the UI thread to sleep here:

Task.Delay(250).Wait();

It's still a low value so it wouldn't matter too much but should you go to higher values I would add some sort of visual feedback so the user can distinguish between the AI "thinking" and the program crashing.

Random

You're creating a Random object inside a method. This is a dangerous approach because it uses a time-based seed so if there are 2 very close method calls, there will not have enough time passed. Consider creating an instance once on class level and calling .Next() on that field.

You're actually using several Random instances throughout your code so they can all benefit from this.

Conclusion

Overall the code seems pretty nice. There's still quite a bit of duplication and I think abstracting a few concepts in classes and interfaces might be a solution to remove some duplication, but overall it is good. The main remarks are style-wise.

StackExchange Code Review Q#47861, answer score: 6