2
\$\begingroup\$

I'm learning C# and currently having an assignment on Tic Tac Toe. I'd love to have some feedback on where I can improve.

I've managed to create the end game functions but I feel it could be improved. I'm just not sure how to do it and was hoping someone here could help me.

I hope the information I've provided is enough, this.cells is a multidimensional array containing the Cell objects:

/// <summary>
/// Contains info about what kind of cell we're dealing with
/// </summary>
public enum Cell {
  Empty,
  Circle,
  Cross
}

Below is the part I'm searching feedback on. Too many if statements? What could I have done different or better?

/// <summary>
/// Check all possible combinations for the target cell
/// of winning the game (row, column, or slope).
///
/// Returns true if there is a win condition, false if there isn't.
/// </summary>
private bool IsGameOver(Cell cell) {
    // Row 1
    if (this.cells[0, 0] == cell && this.cells[0, 1] == cell && this.cells[0, 2] == cell) {
        return true;
    }

    // Row 2
    if (this.cells[1, 0] == cell && this.cells[1, 1] == cell && this.cells[1, 2] == cell) {
        return true;
    }

    // Row 3
    if (this.cells[2, 0] == cell && this.cells[2, 1] == cell && this.cells[2, 2] == cell) {
        return true;
    }

    // Clm 1
    if (this.cells[0, 0] == cell && this.cells[1, 0] == cell && this.cells[2, 0] == cell) {
        return true;
    }

    // Clm 2
    if (this.cells[1, 0] == cell && this.cells[1, 1] == cell && this.cells[1, 2] == cell) {
        return true;
    }

    // Clm 3
    if (this.cells[2, 0] == cell && this.cells[2, 1] == cell && this.cells[2, 2] == cell) {
        return true;
    }

    // Horizontal line 1
    if (this.cells[0, 0] == cell && this.cells[1, 1] == cell && this.cells[2, 2] == cell) {
        return true;
    }

    // Horizontal line 2
    if (this.cells[0, 2] == cell && this.cells[1, 1] == cell && this.cells[2, 0] == cell) {
        return true;
    }

    return false;
}
\$\endgroup\$
4
\$\begingroup\$

Naming

The enum Cell defines Empty, Circle and Cross. This values aren't cells in terms of an object, they are more like types.

For example: Empty is not a cell, but a cell can be empty

You can rename Cell to CellType to make it more clear.

Comments

Comments are a good documentation. You did a great job with the <summary>-tag. But the comments like // Row 1, // Row 2, // Clm 3 are not so good..

Robert C. Martin, who wrote the book "Clean Code" and many more, sad

Don’t Use a Comment When You Can Use a Function or a Variable

Introduce Methods

We can wrap the if-statements into methods

bool IsFirstRowComplete(CellType cellType) {
    if (this.cells[0, 0] == cellType && this.cells[0, 1] == cellType && this.cells[0, 2] == cellType) {
        return true;
    }
    return false;
}

bool IsSecondRowComplete(CellType cellType) {
    if (this.cells[1, 0] == cellType && this.cells[1, 1] == cellType && this.cells[1, 2] == cellType) {
        return true;
    }
    return false;
}

// ...

Make them less complex

Since these methods return a Boolean we can return the boolean-expression itself

bool IsFirstRowComplete(CellType cellType) {
    return this.cells[0, 0] == cellType
           && this.cells[0, 1] == cellType
           && this.cells[0, 2] == cellType
}

bool IsSecondRowComplete(CellType cellType) {
    return this.cells[1, 0] == cellType
           && this.cells[1, 1] == cellType
           && this.cells[1, 2] == cellType
}

// ...

Remove Duplication

The new methods IsFirstRowComplete and IsSecondRowComplete looks very similar. We can extract the main logic and wrap it into its own method.

bool IsRowComplete(CellType cellType, int row) {
    return this.cells[row, 0] == cellType
           && this.cells[row, 1] == cellType
           && this.cells[row, 2] == cellType
}

// ...

If you like you can now wrap this method into IsFirstRowComplete and so on

bool IsFirstRowComplete(CellType cellType, int row) {
    return IsRowComplete(cellType, 0);
}

// ...

Introduce more Methods

We could finish here.. But we can make the code more clear if we wrap each check into a method

bool IsARowComplete(CellType cellType) {
    return IsFirstRowComplete(cellType, 0)
           || IsSecondRowComplete(cellType, 1)
           || IsThirdRowComplete(cellType, 2);
}

bool IsAColumnsComplete(CellType cellType) {
  return IsFirstColumnComplete(cellType, 0)
         || IsSecondColumnComplete(cellType, 1)
         || IsThirdColumnComplete(cellType, 2);
}

// ...

After the Refactoring

bool IsGameOver(CellType cellType) {
    return IsARowComplete(cellType)
           || IsAColumnComplete(cellType)
           || IsAHorizontalComplete(cellType)
}

Maybe a Bug

If the method isGameOver gets called with a CellType of type Empty it could return true

So maybe you need to check if the cellType is not Empty

\$\endgroup\$
  • \$\begingroup\$ Roman thank you so much for your time! I learned so much! <3 \$\endgroup\$ – jubibanna Feb 20 at 11:51
  • \$\begingroup\$ Well done! I'd have gone with CellState though =) \$\endgroup\$ – Mathieu Guindon Feb 21 at 1:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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