Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've tried to take the shortest, simplest path to a C# solution to the Ultimate Tic-Tac-Toe challenge. This implementation plays in the console. I admit the game logic could be hived off into a separate method, but I'm not sure that would be an improvement. I also admit that the fields should be tighter than public, but that's neither here nor there.

I would appreciate feedback on

  1. how readable people find this solution
  2. any suggestions to make the code more succinct
  3. any other comments people would care to make

void Main()
{
    var mb = new MetaBoard();
    var board = -1;
    var player = Who.X;
    while (WinFor(mb) == Who.Neither && mb.Boards.Any(x => WinFor(x) == Who.Neither)) {
        mb.Write();
        Console.WriteLine(player + " to play.");
        // Choose a board if we've just started or the current board is finished.
        while (board == -1 || mb[board] != Who.Neither) board = InputNum("an unfinished board");
        var cell = -1;
        // Choose an empty cell on the board.
        while (cell == -1 || mb.Boards[board][cell] != Who.Neither) cell = InputNum("an empty cell on board " + (board + 1));
        Console.WriteLine();
        mb.Play(board, cell, player);
        // The next board is decided by the cell just played.  Swap turns between the players.
        board = cell;
        player = (player == Who.X ? Who.O : Who.X);
    }
    mb.Write();
    Console.WriteLine(WinFor(mb) + " wins!");
}

static int InputNum(string what) {
    var i = -1;
    do {
        Console.Write("Choose " + what + " [1-9]: ");
    } while (!int.TryParse(Console.ReadLine().Trim(), out i) || i < 1 || 9 < i);
    return i - 1;
}

enum Who { Neither, X, O }

static int[][] Lines = new int[][] {
    new int[] {0,1,2}, new int[] {3,4,5}, new int[] {6,7,8}, 
    new int[] {0,3,6}, new int[] {1,4,7}, new int[] {2,5,8}, 
    new int[] {0,4,8}, new int[] {2,4,6}
};

interface IBoard { Who this[int i] { get; } } // Indicate who, if any, has won position i.

static Who Min(Who p, Who q) { return p == q ? p : Who.Neither; }

static Who WinFor(IBoard board) { // X if a line of three Xs, O if a line of three Os.
    return Lines.Select(line => Min(board[line[0]], Min(board[line[1]], board[line[2]])))
                .FirstOrDefault(who => who != Who.Neither);
}

class Board: IBoard { // A 3x3 grid of cells.
    public Who[] Cells = new Who[9];
    public Who this[int i] { get { return Cells[i]; } }
    public void Play(int i, Who who) { Cells[i] = who; }
}

class MetaBoard: IBoard { // A 3x3 grid of boards.
    public Board[] Boards = Enumerable.Range(0, 9).Select(i => new Board()).ToArray();
    public Who this[int i] { get { return WinFor(Boards[i]); } }
    public void Play(int i, int j, Who who) { Boards[i].Play(j, who); }

    public void Write() {
        Action<Who> writeCell = w => { Console.Write(w == Who.X ? 'X' : w == Who.O ? 'O' : '.'); };
        var rg = Enumerable.Range(0, 3);
        foreach (var rr in rg) foreach (var r in rg) foreach (var cc in rg) foreach (var c in rg) {
            writeCell(Boards[cc + 3 * rr].Cells[c + 3 * r]);
            if (c == 2) Console.Write("  ");
            if (c == 2 && cc == 2) Console.WriteLine();
            if (c == 2 && cc == 2 && r == 2) Console.WriteLine();
        }
    }
}
share|improve this question

4 Answers 4

This doesn't strike me as particularly readable, although it is definitely succinct. Here are a few of the things I noticed that I think could be improved:

  • The name Min() for a method that determines if someone has filled a row is very confusing.
  • WinFor() should be a method on IBoard rather than a static method that takes an IBoard.
  • The variable names in MetaBoard.Write()are also confusing and something like outerRow and innerColumn would be more informative that rr and c.
  • The quadruple foreach loop in the MetaBoard.Write() scares me. I realize what it is doing, but that many nested loops is a code smell. You might want to create a method on IBoard that prints out board contents line-by-line so that you can append them to each other and control when you insert padding and newlines.
  • If you're going to do that, you may consider extracting out a BoardPrinter class or something similar to encapsulate that behavior.
  • The size of the board is hard-coded and although it isn't a requirement, redesigning this to support different-sized boards will make a more flexible design. (I got this idea in a review for my Ultimate Tic-Tac-Toe entry and am happy I re-worked it).

    This will require removing Lines and calculating them manually. As a side note, indices for diagonals can be calculated by consecutively adding the (board width + 1) for downward-right and the (board width - 1) for downward-left diagonals. It took me a while to figure that out.

  • The use of Board[] as the type for the MetaBoard.Boards property indicates to me that the IBoard interface is not as useful as it should be (since MetaBoard is tightly coupled to Board). If you pull out the printing so that it can be done line-by-line as I suggested earlier, you could go back to using IBoard. If done carefully, you could nest MetaBoards within MetaBoards (for the Ultimate in Ultimate Tic-Tac-Toe).
  • I don't see where you restrict players from selecting an already completed board.

I know I'm basically ruining the brevity if you implement my comments, but I am impressed with how you managed to keep it so short. It was fun to review, thanks for sharing!

share|improve this answer
    
many thanks for your comments. Just a few notes in response. WinFor is generic across IBoards, there's no need to reproduce that code on a per-instance or a per-class basis (I'm more of a functional programmer, so my style is a bit different from your usual OO type). MetaBoard.Write is very tight; there's a sweet-spot trade-off between brevity and verbosity -- here I'm trying to channel the likes of K&R. I certainly could (easily) generalise the board size and game depth, but that would violate my principle of heading straight for the target. –  Rafe Feb 7 at 3:08
    
Two more points in response. IBoard is exactly what you need if you're going to generalise this to meta-meta-tic-tac-toe, etc. At the top-level you do need to know the depth of the game because you potentially have to select boards all the way down! The condition preventing the user from selecting an already completed board is while (... || mb[board] != Who.Neither) board = InputNum(...). mb[board] denotes who, if anybody, is the winner of board. Thanks again! –  Rafe Feb 7 at 3:13

I have fully embraced Clean Code's goal of five-line methods, and my life of reading code has absolutely improved as a result. Right at the start I see this and am stumped:

while (WinFor(mb) == Who.Neither && mb.Boards.Any(x => WinFor(x) == Who.Neither)) {... 

This is begging for

while (noOneHasWon() && noOneHasWonAnyBoard())... 

Now clearly the second method name is not correct because you don't end the game with the first miniboard win. But that's the beauty of extracting methods: incomprehensible code and possibly incorrect comments are replaced with easily-refactored method names. Mistakes in reading and coding are more readily apparent because they lie closer together.

Doing that for every three-to-five lines of code allows you to delete all those incorrect and misleading comments while improving readability. Yes, the code might be a little longer, but it will be much faster to read and verify.

share|improve this answer
    
Hi David, agreed: I am guilty of not taking that procedure very seriously here. When you mention "incorrect and misleading comments" are you referring to my comments or commenting in general? –  Rafe Feb 7 at 4:24
1  
@Rafe Commenting in general. The compiler will complain if you change a method name without fixing all calls to it, but it won't if your comments become incorrect. Granted, it won't complain if a method implementation no longer matches it's name, but that's far easier to keep in sync than comments IMHO. –  David Harkness Feb 7 at 4:41
    
absolutely, but comments are the ideal place to describe intention. –  Rafe Feb 7 at 4:54
    
Regarding the line above: I believe you have not taken into consideration the posibility of a tie! –  Andris Feb 7 at 7:34
2  
@Rafe Yes! The code describes what and how and when; comments cover who and why. ;) –  David Harkness Feb 7 at 9:53

As the other commenters have noted, the solution is pretty decent but could be more readable. I won't reiterate the points made by others already. A few short points:

  • new int[] {0,1,2}, and so on can be made less verbose. new[] {0, 1, 2} is legal since C# 3. The type of the array will be inferred from the contents.

  • Suppose I told you that I wrote a program with variables r, rr, c and cc. What would you suppose that program does? Any clue? Me neither.

  • I don't like the IBoard abstraction. The game is simple enough to not require an interface and a hierarchy. Your comment about generalizing to higher levels of meta-ness is a great example of premature design for generality. An excess of generality can impede understanding, is expensive to implement, and can impede the ability to edit the code later. YAGNI -- You Ain't Gonna Need It -- so don't implement it.

  • Questions ("who?") are not great names for types. "Player" would be better.

  • Consider using nullables to represent quantities that can be invalid values, rather than magic constants like -1. (Though keep in mind if you do that it can be unclear whether null means "has a valid value but I don't know what it is" and "has no valid value at all". Is null more like "the sales figures for November, which are a specific value, I just don't know what it is" or "the name of the present king of France"?)

share|improve this answer
    
thanks for your comments. Re: rr etc., they're just small-context iteration variables; I guess metaRow, row, etc. might be better names. Had there just been r and c in this context, would that still be confusing? Definitely agree on IBoard, looking again at my code: it takes up way more space than it saves. Re: nullables, I did consider them. I do wish it were possible to use general algebraic data types in C#: nulls are horrid and, often, objects are too clumsy. –  Rafe Feb 7 at 6:39
2  
+1 for "premature design for generality" –  Andris Feb 7 at 7:05

Bugs:

If we have a tie one of the Borads the game will accept/force me to play on it even if all cells are occupied. Also the game won't finish if all broads are finished but there is at least one tie board on the MetaBoard. So there should be a distinction between unfinished board and tied board.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.