Skip to main content
3 of 3
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/

This reminds me of what I wrote a while ago for my Tic Tac Toe Ultimate game.

In case I get anything wrong, here is a relevant part of my original code:

// Scan diagonals for a winner: Bottom-right for (int yy = 0; yy < board.getSizeY(); yy++) { newWin(conds, consecutive, loopAdd(board, 0, yy, 1, 1)); } private static List<Winnable> loopAdd(HasSub<? extends Winnable> board, int xx, int yy, int dx, int dy) { List<Winnable> winnables = new ArrayList<>(); Winnable tile; do { tile = board.getSub(xx, yy); xx += dx; yy += dy; if (tile != null) winnables.add(tile); } while (tile != null); return winnables; } 

When making this in C#, you can return IEnumerable<char>. The idea of the method is to start at a specific point and to iterate until you're outside the valid area.

I think it's a bad idea that you're currently using String concatenation first, and then checking if the string contains the char. Instead you can indeed as Carsten points out in the comments use a state machine for that. It only needs to be about an int really that keeps track of how many consecutive matches you've had so far, and when it reaches a non-match it starts over at zero. When it reaches the string length, it returns true.

Anyways, back to the main part, here's what I would try to do:

private IEnumerable<char> loopOver(string[] board, Point position, Point delta) { char value; while (true) { if (!charIsInsideBoard(board, position)) break; value = getCharInBoard(board, position); position.x += delta.x; position.y += delta.y; yield return value; } } 

Whenever I'm using C#, I love using the wonderful yield keyword.

private boolean contains(IEnumerable<char> loop, string searchFor) { int matches = 0; foreach (char ch in loop) { if (ch == searchFor[matches]) { matches++; } else { matches = 0; if (ch == searchFor[matches]) { matches++; } } if (matches == searchFor.Length) { return true; } } return false; } 

Then in your allDiagonals method (which really could be renamed to searchAllDiagonals) all you need to do is to call the contains and loopOver methods a couple of times.

boolean searchAllDiagonals(string[] board, string searchFor) { boolean result = false; for (int i = 0; i < board[0].Length; i++) { result = result || contains(loopOver(board, new Point(i, 0), new Point(1, 1), searchFor); } for (int i = 0; i < board.Length; i++) { result = result || contains(loopOver(board, new Point(0, i), new Point(1, 0), searchFor); } ... } 

Please note that I have not tested this code, and as I haven't done any C# development since last time I might have mixed something up. I also might have mixed up X and Y, but that happens for me in Java too every now and then.

Other Suggestions:

  • Don't use i and j when you're using nested loops. row and col are so much better, or x and y. I myself use xx and yy sometimes for loops when I already have another x / y variable.

  • As your variable is named board, what do I know, perhaps your whole point is to check for a winner in a Tic Tac Toe game. If a game is your context, then I'd recommend not using string[]. Use a BoardField[][] variable instead, or preferably wrap such a 2D array in a Board class (which can contain the charIsInsideBoard and getCharInBoard methods).

Simon Forsberg
  • 59.8k
  • 9
  • 160
  • 312