Check for a win on the board This code is part of a tic tac toe game. It uses a 2D array for the board.

The functionality of the following code is to check for a win on the board, in rows, columns and diagonals. How can I improve it?

public bool WhoWon(Cell cell) 
{
    for (int i = 0; i &lt; 3; i++) 
    {
        for (int j = 0; j &lt; 3; j++) 
        {
            if (_board[i, 0] == cell &amp;&amp; _board[i, 1] == cell &amp;&amp; _board[i, 2] == cell) 
            {
                return true;
            } 
            else if (_board[0, j] == cell &amp;&amp; _board[1, j] == cell &amp;&amp; _board[2, j] == cell) 
            {
                return true;
            }
        }
    }

    if (_board[0, 0] == cell &amp;&amp; _board[1,1] == cell &amp;&amp; _board[2,2] == cell) 
    {
        return true;
    }
    else if (_board[0, 2] == cell &amp;&amp; _board[1, 1] == cell &amp;&amp; _board[2, 0] == cell) 
    {
        return true;
    }
    return false;
}
What you've done here is listed all possible checks, but you've compiled it into a single method. Ideally, you should split this up into separate tasks, as there are three categories of similar checks:


Horizontal checks
Vertical checks
Diagonal checks


Maybe the current example is just simple enough to not fully warrant it, but especially if you had had many different win conditions (e.g. another rule that states making a 2x2 square also counts as a win), you'd understand why you need to break your validation into smaller (logically coherent) parts.



The method


public bool WhoWon(Cell cell) 



I agree with the other answers here. The name of the method does not match its return type. WhoWon should return a player. HasWon should return a boolean.

I'm also not a fan of the "cell" name. Looking at the code, cell seems to contain the player's letter (X or O).  That is not immediately apparent by calling it a cell, a cell would be a box on the board.

E.g. board[1,1] is a cell, as it represents a box on the board. But I would call the value of board[1,1] a token, i.e. the symbol that is contained within (if it's not empty).

You haven't shared the how Cell is defined. This may already be close to what I'd prefer to use. Based on this, let me suggest an improvement:

public enum Token { X, O, Empty }

public Token[,] _board = new Token[3,3];

public bool HasWon(Token token)
{
    // ...
}


Notice that you could also implement WhoWon() based on HasWon():

public Token WhoWon()
{
    if(HasWon(Token.X)) 
        return Token.X;

    if(HasWon(Token.O)) 
        return Token.O;

    return Token.Empty;
}




The method body

I suggest splitting this up, as it will dramatically simplify the validation logic and make it easier to parse.

public enum Token { X, O, Empty }
public Token[,] _board = new Token[3,3];

public bool HasWon(Token token) 
{
    return
        IsHorizontalVictory(token)
        || IsVerticalVictory(token)
        || IsDiagonalVictory(token);

}

private bool IsHorizontalVictory(Token token)
{
    for(int y = 0; y &lt;= 2; y++)
    {
        if(_board[0,y] == token &amp;&amp; _board[1,y] == token &amp;&amp; _board[2,y] == token)
            return true;
    }

    //No victory condition hit = no victory.
    return false;
}

private bool IsVerticalVictory(Token token)
{
    for(int x = 0; x &lt;= 2; x++)
    {
        if(_board[x,0] == token &amp;&amp; _board[x,1] == token &amp;&amp; _board[x,2] == token)
            return true;
    }

    //No victory condition hit = no victory.
    return false;
}

private bool IsDiagonalVictory(Token token)
{
    if(_board[0,0] == token &amp;&amp; _board[1,1] == token &amp;&amp; _board[2,2] == token)
        return true;

    if(_board[0,2] == token &amp;&amp; _board[1,1] == token &amp;&amp; _board[2,0] == token)
        return true;

    //No victory condition hit = no victory.
    return false;
}


Comments:


You could further abstract this, e.g. by making the dimensions of the board variable. However, TicTacToe is always played on a 3x3 board, so this seems unnecessary at the moment.
Instead of using a multidimensional array, you could create a full OO representation of the board with its cells. This is going to cost some effort to put together, but it will make board operations (such as checking for victory) nicer to read. Index-based retrieval is nice when simple, but can become horrendously complex to follow if the logic gets to be too big.
Some things could have been rewritten, e.g. the IsDiagonalVictory() method could have merged the two checks (+ the false return value) into one boolean: return (logic_for_diag_1) || (logic_for_diag_2). However, this would've storngly affected the readability of the code. As the performance difference is negligible, I therefore chose the most readable option.




This was a headscratcher


for (int i = 0; i &lt; 3; i++)  //A
{
    for (int j = 0; j &lt; 3; j++)   //B
    {
        //A
        if (_board[i, 0] == cell &amp;&amp; _board[i, 1] == cell &amp;&amp; _board[i, 2] == cell) 
        {
            return true;
        } 

        //B
        else if (_board[0, j] == cell &amp;&amp; _board[1, j] == cell &amp;&amp; _board[2, j] == cell) 
        {
            return true;
        }
    }
}



It took me a while to understand what your intention was here. You've combined two completely separate for-loops, and have for some reason decided to intwine them.

I marked them A and B in the code, because these two are completely separate pieces of logic. Splitting them up:

for (int i = 0; i &lt; 3; i++)  //A
{
    if (_board[i, 0] == cell &amp;&amp; _board[i, 1] == cell &amp;&amp; _board[i, 2] == cell) 
    {
        return true;
    }         
}

for (int j = 0; j &lt; 3; j++)   //B
{
    if (_board[0, j] == cell &amp;&amp; _board[1, j] == cell &amp;&amp; _board[2, j] == cell) 
    {
        return true;
    }
}


You may be wondering, does it matter?

Yes, it does. First of all, splitting them dramatically enhances the readability of the code.

Secondly, you're going to end up doing less loops.


Nested for-loops are multiplicative. 3 loops for A times 3 loops for B = 9 loops. Furthermore, each of the 9 loops is doing two checks, thus resulting in 18 checks occurring.
Separate for-loops are additive. 3 loops for A plus 3 loops for B = 6 loops. Each loop only does one check, thus resulting in 6 checks occuring.


Your nested loops were performing every unique check three separate times (18 / 6 = 3), which serves no functional purpose.

Let's look at only the vertical check:


if (_board[i, 0] == cell &amp;&amp; _board[i, 1] == cell &amp;&amp; _board[i, 2] == cell)



This was being executed for every pair of i,j, while you only needed to test for every value of i.

This is a comparison of the pairs (i,j) and what the vertical check actually did:

(0,0)   =&gt;   Checks vertical column 0
(0,1)   =&gt;   Checks vertical column 1
(0,2)   =&gt;   Checks vertical column 2
(1,0)   =&gt;   Checks vertical column 0
(1,1)   =&gt;   Checks vertical column 1
(1,2)   =&gt;   Checks vertical column 2
(2,0)   =&gt;   Checks vertical column 0
(2,1)   =&gt;   Checks vertical column 1
(2,2)   =&gt;   Checks vertical column 2


Notice how it repeats the same thing for every value of j (0,1,2), even though the value of j has no impact on the outcome of the result.

This is a comparison of the values of i and what the vertical check actually did:

(0)   =&gt;   Checks vertical column 0
(1)   =&gt;   Checks vertical column 1
(2)   =&gt;   Checks vertical column 2


By not observing the value of j, we haven't repeated the same check for every distinct value of j.