Biggest syntax suggestion
There's a lot of repetition and naming issues that makes the code hard to read. For example, Player#valid_move? could instead be (ignoring the logic correctness of logic):
def valid_move?(from_square, to_square, piece)
is_pawn = piece.is_a?(Pawn) # not sure why of all piece types, pawns are specifically being singled out here
same_x = to_square.x == from_square.x # not sure why only x is being checked
dest_occupied = !!to_square.piece_on_square
land_on_enemy_piece = dest_occupied && to_square.piece_on_square.color == piece.color # give a name to this "concept"
if is_pawn && !same_x && land_on_enemy_piece
piece.get_valid_captures(from_square, to_square)
elsif !is_pawn || (same_x && !dest_occupied)
piece.get_valid_moves(from_square, to_square)
else
false
end
end
Biggest OOP design suggestion
Pieces should be as dumb as possible
Pieces ought to define how they move, but they shouldn't be able to Piece.get_valid_moves. Determining valid moves requires a few things:
- How a piece generally moves
- The piece's position on the board
- State of the board/where other pieces are
- Whether pieces in their path are allies or enemies
If Pieces can determine valid moves, they'd need to "know" nearly everything on the board. This defeats the purpose of OO Encapsulation! If pieces are "dumb" and well-encapsulated, then Piece is a lower level abstraction and Board and Player depends on Piece, not the other way around.