Skip to main content
deleted 1 character in body
Source Link
Kache
  • 356
  • 1
  • 10

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.

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):

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.

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 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.

added 5 characters in body
Source Link
Kache
  • 356
  • 1
  • 10

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):

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 a Piece is a lower level abstraction thatand Board and Player depends on Piece, not the other way around.

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):

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 a Piece is a lower level abstraction that Board and Player depends on, not the other way around.

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):

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.

Source Link
Kache
  • 356
  • 1
  • 10

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):

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 a Piece is a lower level abstraction that Board and Player depends on, not the other way around.