Help eliminating friends/internals

Question

I often see people say things like "if you need friend/internal then your design is wrong", could someone tell me how to redesign the following code to eliminate the internal in ChessPiece.Location?

It's currently used so that adding a piece to ChessBoard sets the ChessPiece.Location property to match, obviously making it public would be even worse than internal and making it private would prevent ChessBoard from updating the Location. Thanks for any insights.

public struct Coord
{
   public Coord(int x, int y) { this.X = x; this.Y = y; }
   public int X { get; private set; }
   public int Y { get; private set; }
}

public class ChessBoard
{
   public ChessBoard() { /*[...]*/ }
   public ChessPiece this[int x, int y]
   {
       get
       {
           // Return ChessPiece at this position (or null)
       }
       set
       {
           // Add ChessPiece at this position and set its Location property
       }
}

public class ChessPiece
{
   public ChessPiece() { /*[...]*/ }
   public Coord Location { get; internal set; }
}

Answer 1

My first thoughts would be

• board is just a container
• piece has a public read-only immutable position property
• Initial position is set in piece construction
• Position is changed by invoking a move operation on a piece

Answer 2

I personally think it's strange that the ChessPiece knows it's location - that seems like a function of the ChessBoard, not the piece itself. (What location is a chess piece after it's taken and off the table? It'd normally still be a valid piece...)

I'd put the location/movement logic into the ChessBoard, and store a Dictionary<ChessPiece, Coord> for the location of each valid chess piece.

Answer 3

I often see people say things like "if you need friend/internal then your design is wrong"

I think that's silly. Friend and internal exist in the language for good reason.

eliminate the internal in ChessPiece.Location

That would make it impossible to update the ChessPiece.Location.

That would be OK if the ChessPiece knew enough about the game to update its own Location, for example:

public class ChessPiece
{
  public ChessPiece() { /*[...]*/ }
  public Coord Location { get; }
  //a measure of how good it would be to move this piece
  public int GoodnessOfBestMove
  {
    get
    {
      //calculate what self's best possible move is
      ... todo ...
    }
  }
  //an instruction to go head and do that move
  public void Move()
  {
    //do self's self-calculated best move, by updating self's private Location
    ... todo ...
  }
}

class Strategy
{
    void Move()
    {
      ChessPiece bestChessPiece = null;
      foreach (ChessPiece chessPiece in chestPieces)
      {
        if ((bestChessPiece == null) ||
          (chessPiece.GoodnessOfBestMove > bestChessPiece.GoodnessOfBestMove))
        {
          //found a better piece to move
          bestChessPiece = chessPiece;
        }
      }
      //found the best piece to move, so now tell it to move itself
      bestChessPiece.Move();
    }
}

For further details there's an OO principle called "tell don't ask" which you can Google for.

Another possibility is for the Board to store each ChessPiece in a 2-D array owned by the board. To move a piece, the Board puts the piece in a different slot in the array; and when a piece wants to report its location, it searches for itself in the array.

ALthough the above are alternatives, I'm not saying that they're necessarily good or better solutions for this problem: I don't think I'd want to encode game strategy as an implementation detail of each piece; instead I might have some non-private way to let some other class update the Location of each piece.

Answer 4

I agree strongly with Steve. Internal and friend are in the language for a reason, but this isn't necessarily an example of it.

Personally I'd put a Move(Coord) method on the piece. That way the piece can validate the validity of its own moves (plus you can use derived classes for specialized logic, i.e. for pawn vs knight). Coord should be immutable (the coordinate doesn't move and should be immutable, the piece moves to a new coordinate).

You shouldn't be able to just set a piece on the board at any point, that moves your responsibility for the validity of a move to your calling class. If you need to set up custom boards, create a new board with a new starting position.

I'd also disagree about the strategy approach. I do agree a strategy pattern is good here, but it shouldn't necessarily be built into the piece. I would move the strategy into piece-specific strategy classes. This way your pieces and board are simple, clean and usable for a person-vs-person game and extended for a person-vs-cpu game