Overland Chess Review

overland_chess_review.md

Overview

So this will mostly be about the things I think can be improved, but I did want to point out that I think there are some cool ideas here, I just don't talk about them much in this review because it'd take way more writing. The Evaluation class is a really neat idea that I think fits with your code very well.

Also obviously a huge accomplishment in general to complete this, and no one's design is perfect (least of all my own). With that said, I'll jump right in

Functionality

Other than the bugs you mentioned, everything seems smooth save this one problem for castling to the queen side, where the participating rook isn't moved. I didn't immediately notice any other bugs going on, which is great!

I'd probably like some sort of game message after stalemate, similar to what you have with checkmate. Right now, the game just kinda ends without telling the player anything about what happened.

Design

Piece & Piece Hierarchy

Folder Structure

It's common convention with inheritance hierarchies to group the subclasses in their own directory. So at the top level of lib/ you'd have piece.rb, where the superclass is defined, and then a pieces/ folder would hold all of Piece's subclasses. You'd mirror this structure in your spec/ folder as well, with piece_spec.rb at the top level and then a pieces/ folder to hold all the subclass spec files.

Here's an example tree from my own chess project if you need visualization.

Polymorphism & Inheritance

Your Piece super class is very light. It defines some attr_ macros and a factory function.

This class is what's called an "abstract superclass" -- meaning you don't ever intend to instantiate it directly. Such classes can be very useful for sharing behavior among subclasses, enforcing certain methods to exists on subclasses, and creating opportunities for "polymorphism".

Now I think I've seen you say recently in one of the ruby chats that you don't know what polymorphism really is, but it's essentially just a scary word for allowing different types of things to present a single, unified interface. Other things could interact with the polymorphic structure without knowing any concrete details. Let's look at an example from your code.

promote_pawn if chosen_piece.is_a?(Pawn) && chosen_piece.can_be_promoted?

Here the code that exists outside of the Piece hierarchy has to concern itself with some detail -- is the thing I'm talking to an instance of Pawn? I posit that this code should be unconcerned with whether or not it's talking to a Pawn, and should instead be able to function purely from this:

promote_pawn if chosen_piece.can_be_promoted?

But how? By letting other pieces have the same interface. Every other piece can have this method, and they all have a pretty simple and straightforward reply to it in all circumstances: false. And because this can be considered a type of "default" that only the Pawn handles differently, we can put it in the super and let others inherit it.

class Piece
  # other stuff

  def can_be_promoted?
    false
  end
end

# and then in Pawn:
class Pawn
  def can_be_promoted?
    # overrides the super's method with its own logic
  end
end

This is making your Piece hierarchy more polymorphic, which is a very good thing. This was a simple example, but following this idea can really clean up how other code interacts with Piece and its subclasses. They just tell the pieces what to do in a generalized manner and the pieces react appropriately.

You can think of this as really the same thing you naturally did with #to_s. When you're creating the display logic out, you're not having to do something like:

if piece.is_a?(Knight)
  unicode = # knight unicode
elsif piece.is_a?(Pawn)
  unicode = # pawn unicode
# etc.

But with to_s on each piece, you can just get any piece's string representation in a completely straightforward way without other code having to care about whether it's a Knight or a Rook or whatever.

Board

Conditional Logic

Complex conditional logic can be pretty dang tough to reason about, and you pretty much always want to think of ways to get rid of it when it crops up. Let's look at this method:

def add_background_color
  @grid_clone = @grid.map(&:clone)
    @grid_clone.each_with_index do |row, idx|
      if idx.even?
        row.each_index do |idx|
          piece = row[idx]
          row[idx] = TAN_BG + " #{piece ? piece : ' '} " + RESET_CODE if idx.even?
          row[idx] = GREEN_BG + " #{piece ? piece : ' '} " + RESET_CODE if idx.odd?
        end
      else
        row.each_index do |idx|
          piece = row[idx]
          row[idx] = TAN_BG + " #{piece ? piece : ' '} " + RESET_CODE if idx.odd?
          row[idx] = GREEN_BG + " #{piece ? piece : ' '} " + RESET_CODE if idx.even?
        end
      end
    end
  end
end

First of all, any particular reason you don't pay more attention to rubocop? You definitely don't have to listen to it freak out on methods that are over 10 lines long, but often it has some very good advice. The variable shadowing it flags here is something that can be legitimately confusing for a reader:

@grid_clone.each_with_index do |row, idx|
  if idx.even?
    row.each_index do |idx|

So now there are two variables named idx that are in scope. The code still works because it lets the last one defined take over, but these name collisions can make your code confusing. Simplifying the ternary with piece is also nice rubocop advice:

"#{piece ? piece : ' '}"

# could be->
"#{piece || ' '}"

And now with the conditionals, it can be helpful to map out the different branches. I write out a little chart to help illuminate something that may not be clear from thinking of it just in my head.

	even row	odd row
even col	TAN	GREEN
odd col	GREEN	TAN

So if they're both even/odd we want one result, and if they're different, we want the other. So this check can be simplified down to just checking that the two values don't have the same evenness (or oddness). If they don't, it's a green square, and if the do, it's a tan square.

(2.even? != 3.even?)
#-> true

BoardToFEN

Distribution of Conditional Logic

def piece_to_fen(piece)
  case piece
  when Rook
    piece.color == :white ? 'R' : 'r'
  when Bishop
    piece.color == :white ? 'B' : 'b'
  when Knight
    piece.color == :white ? 'N' : 'n'
  when Queen
    piece.color == :white ? 'Q' : 'q'
  when King
    piece.color == :white ? 'K' : 'k'
  when Pawn
    piece.color == :white ? 'P' : 'p'
  end
end

So with this code, why not give the pieces more control here? It makes sense for a Rook to know it's represented by the letter 'r'. We also simplify the conditional logic up a lot here by dispersing over all the different pieces individually. Again, this is a big advantage with having this inheritance structure in place. This also gets back to the polymorphism thing. We'd rather components outside of Piece get to treat any piece in a generic way.

# an example:
class Piece
  # inherited by every piece
  def to_fen
    self.class::FEN_CHARS[color]
  end
end

class Rook
  FEN_CHARS = { white: 'R', black: 'r' }.freeze

  # other stuff
end

class BoardToFEN
  # arguably don't need this anymore because it's become so simple
  def piece_to_fen(piece)
    piece.to_fen
  end
end

Check

Flag Arguments

This seems to happen a few times in your project. You have a flag argument (aka a boolean), but you're not using keyword arguments to define them. This is another thing rubocop recommends that definitely increases readability. Let's look at an example:

def no_moves_left?(checking_stalemate = false)
  board.pieces(@current_player_color).all? do |piece|
    piece.generate_possible_moves(board)
    piece.possible_moves.length.zero?
  end
end

def stalemate?
  no_moves_left?(true) && !king_in_check?
end

You can see the problem at the point of call. It's not a big deal here because the definition and the point of call are so close together, but just look at how the method reads where it's called:

no_moves_left?(true)

True? True what? It's impossible to understand what that argument is doing without going to look at the method definition.

def no_moves_left?(checking_stalemate: false) # keyword argument
  # stuff
end

# and called like
no_moves_left?(checking_stalemate: true) # now a reader can understand what's happening at the method call

A more pernicious example can be seen with generate_possible_moves, which is called in this module but is defined in a completely different file:

opponent_piece.generate_possible_moves(@board_copy, true) # what is true?

FEN

Factories

So here you're not really leveraging the advantage of the factory function you have for Piece:

def piece_from_fen_char(char, col_number)
  case char
  when 'R'
    col_number = 1 if col_number > 0
    Piece.for(:rook, :white, col_number)
  when 'r'
    col_number = 1 if col_number > 0
    Piece.for(:rook, :black, col_number)
  when 'N'
    Piece.for(:knight, :white, 0)
  when 'n'
    Piece.for(:knight, :black, 0)
  when 'B'
    Piece.for(:bishop, :white, 0)
  when 'b'
    Piece.for(:bishop, :black, 0)
  when 'Q'
    Piece.for(:queen, :white, 0)
  when 'q'
    Piece.for(:queen, :black, 0)
  when 'K'
    Piece.for(:king, :white, 0)
  when 'k'
    Piece.for(:king, :black, 0)
  when 'P'
    Piece.for(:pawn, :white, col_number)
  when 'p'
    Piece.for(:pawn, :black, col_number)
  end
end

There's not much difference in this and saying when 'R' Rook.new() type thing. It'd be really nice to make the Piece factory smart enough to just be able to create the pieces from the character without this big case statement happening here.

The Missing Class

The main thing I think you're missing is a Display class or module. Not having this means your display logic is dispersed throughout your data models.

Think about how much you'd have to change if you wanted to hook this up to a browser front end rather than a terminal one. You'd have to go through and change various methods in several different classes to have it work properly. But if you had a Display class that encapsulates all the display logic, then working with a different UI system would be just creating a different Display class with different functionality and plugging that into your game instead of the terminal one.

This is all to say that the display/UI is its own concern that should be kept separate from business logic and data models. Think about how Rails has the "View" layer of the app separate from the "Model" layer.

Ending

Hopefully this was helpful. I also recommend looking through Roli's Chess if you want a project to look through that implements a lot of the things I mention in this. I'll also definitely answer any questions if you have them.