Is this try pattern to fetch entities from a chess board, easy to follow?

Question

I'm currently writing a game of chess, and, more specifically, I'm creating the code for the game board. The board is simply a static class that contains a multi-dimensional array representing the board, and methods to interact with it. The method in question for the purposes of this question, is called TryGetEntity:

private static GameEntity[,] _board = new GameEntity[8,8];
...
public static bool TryGetEntity(BoardCoordinate requestedPosition, out GameEntity occupyingEntity) {
    occupyingEntity = null;
    if (_board.GetLength(0) < requestedPosition.X || _board.GetLength(1) < requestedPosition.Y)
        return false;
    
    if (_board[requestedPosition.X, requestedPosition.Y] != null)
        occupyingEntity = _board[requestedPosition.X, requestedPosition.Y];
    else return false;
    return true;
}

My concern with this implementation is the default return value of true. I've been taught over many years to assume failure which loosely translates to returning false by default. Currently, I'm at war with myself on refactoring this to:

occupyingEntity = null;
if (_board.GetLength(0) >= requestedPosition.X || _board.GetLength(1) >= requestedPosition.Y) {
    if (_board[requestedPosition.X, requestedPosition.Y] != null) {
        occupyingEntity = _board[requestedPosition.X, requestedPosition.Y];
        return true;
    }
}

return false;

I personally think that the second version reads better, but I'm wanting a second (or more) opinion on it.

---

Edit: Per request from the comments, here is the entire GameBoard class, along with an example use-case (this project is still in very early phases, so no actual calls exist yet):

public static class GameBoard {
    private static GameEntity[,] _board = new GameEntity[8,8];
    public static bool TryGetEntity(BoardCoordinate requestedPosition, out GameEntity occupyingEntity) {
        occupyingEntity = null;
        if (_board.GetLength(0) >= requestedPosition.X || _board.GetLength(1) >= requestedPosition.Y) {
            if (_board[requestedPosition.X, requestedPosition.Y] != null) {
                occupyingEntity = _board[requestedPosition.X, requestedPosition.Y];
                return true;
            }
        }

        return false;
    }
}

// Example use-case:
var requestedCoordinate = new BoardCoordinate(3, 3);
if (GameBoard.TryGetEntity(requestedCoordinate, out GameEntity occupyingPiece))
    CapturePiece(occupyingPiece);
else
    MoveToPosition(requestedCoordinate);

---

Does this implementation of the try pattern, in its originally presented state, seem easy to follow? How about the second version?

Edit: I think, that by posting this question, I sort-of answered it. Given that it made me stop and seek a second opinion in its original state, I suspect the same would be true for others. Additionally, now that I've gotten some sleep, the first version is much harder to read than the second. It's not more complex, just, cluttered and doing too much. I'll keep this question up for others to answer in greater detail, but I'm not going to be surprised if the consensus is "the first method is harder to read".

_{Note: Let's ignore formatting for this post.}

Answer 1

First version:

• You shouldn't omit braces, {}, although they might be optional, because that can lead to hidden and therefore hard-to-track bugs. Having braces helps structuring the code as well.
• Instead of the else you just could return early inside the if block and have a return false; afterwards.

This would result in

public static bool TryGetEntity(BoardCoordinate requestedPosition, out GameEntity occupyingEntity)
{
    occupyingEntity = null;
    if (_board.GetLength(0) < requestedPosition.X || _board.GetLength(1) < requestedPosition.Y)
    {
        return false;
    }

    if (_board[requestedPosition.X, requestedPosition.Y] != null)
    {
        occupyingEntity = _board[requestedPosition.X, requestedPosition.Y];
        return true;
    }
    return false;
}

which is IMO easy to read.

The second version:

Well, I think the nested ifs are hurting readability. I would at least keep the first if separately and would change the ..... no, forget it. I would just take the first version and change it like so

public static bool TryGetEntity(BoardCoordinate requestedPosition, out GameEntity occupyingEntity)
{
    occupyingEntity = null;
    if (_board.GetLength(0) < requestedPosition.X || _board.GetLength(1) < requestedPosition.Y)
    {
        return false;
    }

    occupyingEntity = _board[requestedPosition.X, requestedPosition.Y];
    return occupyingEntity != null;
}

Answer 2

Yes the code is readable.

I'm curious why you feel you need multiple return statements. The return statement can return a boolean expression.

public static class GameBoard
{
    private static GameEntity[,] _board = new GameEntity[8, 8];
    public static bool TryGetEntity(BoardCoordinate requestedPosition, out GameEntity occupyingEntity)
    {
        occupyingEntity = null;
        if (_board.GetLength(0) >= requestedPosition.X || _board.GetLength(1) >= requestedPosition.Y)
        {
            if (_board[requestedPosition.X, requestedPosition.Y] != null)
            {
                occupyingEntity = _board[requestedPosition.X, requestedPosition.Y];
            }
        }

        return occupyingEntity != null;
    }
}

Answer 3

I don't know C#, but this is what Nullable types are for. From what I can tell, they obsolete the TryX+out parameter combination.

So your method signature should be more like:

public static GameEntity? GetEntity(BoardCoordinate requestedPosition)

Where you either return a valid GameEntity directly, or just return null.

Answer 4

Your first version of TryGetEntity essentially follows this structure:

public static bool TryGetEntity(BoardCoordinate x, out GameEntity ret) {
    ret = null;
    if (condition1)
        return false;
    
    if (! condition2)
        ret = value;
    else return false;
    return true;
}

It’s otherwise clean, but this structure is just … weirdly inconsistent. Why not write the conditions as consistent guard clauses with early exits, and put the successful code path at the end (saving one else in the process)?

public static bool TryGetEntity(BoardCoordinate x, out GameEntity ret) {
    ret = null;

    if (condition1)
        return false;
    if (condition2)
        return false;
    
    ret = value;
    return true;
}

This makes the logic much more obvious.

Answer 5

To me the code presented by @pacmaninbw is close to what I would write, but I would simplify it to avoid the second if-statement giving:

public static bool TryGetEntity(BoardCoordinate requestedPosition, out GameEntity occupyingEntity) {
     occupyingEntity = null;

     if (_board.GetLength(0) >= requestedPosition.X || _board.GetLength(1) >= requestedPosition.Y) {
          occupyingEntity = _board[requestedPosition.X, requestedPosition.Y];
     }

     return occupyingEntity != null;
}

I also find that this style of {...} is less impactful making it easier to consistently add it everywhere.

Answer 6

I don't know C# but this is how I would write the code

public static class GameBoard {
    private static GameEntity[,] _board = new GameEntity[8,8];
    public static GameEntity TryGetEntity(x, y) {
        if (_board.GetLength(0) < x && _board.GetLength(1) > y)
            return null;
        
        return _board[x, y];
    }
}

// Example use-case:
GameEntity occupyingPiece = GameBoard.TryGetEntity(3,3);
if (occupyingPiece != null)
    CapturePiece(occupyingPiece);
else
    MoveToPosition(3,3);

Syntax might not be 100% correct, as I said I don't know much C#, but the theme should be the same:

1. Do your checks and return null rather than true/false
2. Check for null on return, better on a variable than on a function
3. Coordinates class seemed redundant

Without seeing your entire code something tells me that your choice of statics is a bit exaggerated.