CODEWITHSUNDEEP
c#classoop
parhine
parhine

Reputation: 123

Is it ok that 2 objects reference each other?

I'm making a chess game in C#. I've got 2 classes, Field and Piece:

public class Field
{
    // the piece that is standing on this field
    // null if no piece is standing on it
    public Piece piece { get; set; }
}

public class Piece
{
    // the field this piece is standing on
    public Field field { get; set; }
}

When a piece moves, this method is called (in class Piece):

public void Move(Field field)
{
    this.field = field;
    field.piece = this;
}

This doesn't seem to be good coding, because everytime I change the field property, I also have to change the piece property for that field. I do need both properties though, because elsewhere in my code, I need them both to do checks etc (e.g. what's the field this piece is on and by what piece is this field taken).

My question: is this completely ok, is it a bad code smell or is it totally wrong? What would be a good solution to solve this?

Any advice? Thanks in advance!

Upvotes: 3

Views: 2261

Answers (2)

Dimitar
Dimitar

Reputation: 4783

No! This relates to concept of circular dependency. Although applied for modules, this may very well be seen as precursor for such.

More concretely, this is an ideal example for mutually recursive objects. Conceptually, if you substitute (semi-pseudocode)

public class Field
{
    public Piece piece {
        public Field field {
            public Piece piece {
                ...
            }
        }
    }
}

That's because the objects are defined in terms of each other. Then theoretically you can do something like

this.field.piece.field.piece...

Upvotes: 0

Rufus L
Rufus L

Reputation: 37020

The problem I see here is that you have Piece.field and Field.piece as public properties. This means that others can set these properties without updating the corresponding one.

Additionally, when you move a piece from one field to another, you don't remove the piece from the previous field, and we allow pieces to move to occupied squares, which will result in multiple pieces referring to the same field, but the field will only refer to the last piece placed there.

To address these, I would make the properties read only (with a private setter), forcing clients to call the corresponding Set or Move method to change them. Then, in this method, we can verify that the field we're moving to is not occupied (if it is, we simply throw an exception - the client must check this first before calling Move), and that we clear the Piece from the Field we moved from.

The validation work can be done in either the Field or Piece class, or both. I put it all in the Field class to simplify things.

Even still, there are problems with this. You can call Field.SetPiece(piece) directly (instead of Piece.MoveTo(field);), which will leave the piece with a null value for Field. So this is only a slight improvement, but not the ideal solution. See below for a better idea.

public class Field
{
    public Piece Piece { get; private set; }
    public bool Occupied => Piece != null;

    public void ClearPiece()
    {
        // Remove this field from the piece
        if (Piece?.Field == this) Piece.MoveTo(null);

        // Remove the piece from this field
        Piece = null;
    }

    public void SetPiece(Piece piece)
    {
        if (piece != null)
        {
            if (Occupied)
            {
                throw new InvalidOperationException(
                    $"Field is already occupied by {Piece}.");
            }

            // Remove piece from the piece's previous field
            if (piece.Field?.Piece == piece)
            {
                piece.Field.ClearPiece();
            }
        }

        Piece = piece;
    }
}

public class Piece
{
    public Field Field { get; private set; }

    public void MoveTo(Field field)
    {
        field.SetPiece(this);
        Field = field;
    }
}

After thinking a little more about this, I think a better solution would be to have a GameManager class that handles all the validation and movement, and then we can make the Field and Piece classes "dumb".

This makes sense because there is a lot more validation to be done before setting a Piece on a Field. Is it ok to move this piece to the location (i.e. if the King is in check and this doesn't block it, then it's not allowed). Is the Field a valid landing spot for the piece based on the piece's move rules (i.e. a horizontal position for a bishop would not be allowed)? Is there anything blocking the path of the piece to get to the destination? Is the destination occupied by another piece belonging to the same player? Many things to evaluate before moving a piece.

Additionally, this would allow us to reuse the Piece and Field classes in other types of games, which may have a different set of rules, and a different GameManager to enforce them.

Upvotes: 2

Related Questions