doubled the performance of check/mate detection by working backwards …

…from king instead of forwards. Added some duplication DRY up next time

trunk/app/models/board.rb

@@ -63,24 +63,20 @@ def allowed_moves( position )
     moves
   end
 
-  #The algorithm for in_check detection is a brute force search through all of your opponents
-  # allowed moves to see whether one of them is the square your king is on. This could perhaps
-  # be optimized to search backward from the king or some other such technique. Nonetheless, this
-  # routine, and even checkmate even more so (since it calls this routine repeatedly) are ripe 
-  # grounds to practice optimization
+  #The algorithm for in_check detection now searches backward from the king for pieces who could 
+  # be attacking it - this makes it unnecessary to search all the opponents possible moves
   def in_check?( side )
-    #for all pieces which are owned by the opponent
-    opponents_positions = keys.select{ |key| self[key] && self[key].side != side }
     king_position = keys.detect{ |key| self[key].side==side and self[key].role==:king }
+
+    return true if pawn_is_attacking_king(side, king_position)
+
+    return true if knight_is_attacking_king(side, king_position)
 
-    #for all their allowed moves, we are in check if even one of them is our kings position
-    opponents_positions.each do |pos|
-      allowed_moves_of_piece_at(pos) do |move|
-        return true if move.to_sym==king_position
-      end
-    end
+    return true if diagonal_piece_is_attacking_king(side, king_position)
+
+    return true if straight_piece_is_attacking_king(side, king_position)
 
-    return false
+    false
   end
 
   #See the notes for in_check as well. The current algorithm for in_checkmate is: do you have a 
@@ -115,6 +111,82 @@ def allowed_moves_of_piece_at( position )
     end
   end
 
+  def pawn_is_attacking_king(side, king_position)
+    upfield = side==:white ? 1 : -1 #the direction an attacking pawn would come from
+    [ [upfield, 1], [upfield, -1] ].each do |possible_pawn_direction|
+        square = Position.new(king_position) + possible_pawn_direction
+        next unless square.valid?
+        return true if self[square] and self[square].side != side and self[square].role==:pawn
+    end
+    false
+  end
+
+  #TODO created some duplication in order to speed up checkmate 5x - DRY it up
+  def diagonal_piece_is_attacking_king(side, king_position)
+    diagonals = []
+    Piece::DIAGONAL_MOTIONS.each { |motion| diagonals << LineOfAttack.new(motion) }
+    diagonals.each do |diagonal|
+      diagonal.each do |vector|
+        square = Position.new(king_position) + vector
+        break unless square.valid?
+
+        #move on down the line if no piece found
+        next unless self[square]
+
+        #otherwise see if validly attacked
+        if self[square].side != side and [:queen, :bishop].include?( self[square].role )
+          #we found an attacker
+          return true
+        else
+          #it was a blocker - ignore the rest of this line
+          break
+        end
+      end
+    end
+    false
+  end
+
+  def straight_piece_is_attacking_king(side, king_position)
+    straights = []
+    Piece::STRAIGHT_MOTIONS.each { |motion| straights << LineOfAttack.new(motion) }
+    straights.each do |straight|
+      straight.each do |vector|
+        square = Position.new(king_position) + vector
+        break unless square.valid?
+
+        #move on down the line if no piece found
+        next unless self[square]
+
+        #otherwise see if validly attacked
+        if self[square].side != side and [:queen, :rook].include?( self[square].role )
+          #we found an attacker
+          return true
+        else
+          #it was a blocker - ignore the rest of this line
+          break
+        end
+      end
+    end
+    false
+  end
+
+  def knight_is_attacking_king(side, king_position)
+    Piece::KNIGHT_MOVES.each do |vector|
+        square = Position.new(king_position) + vector
+        next unless square.valid?
+
+        #move on down the line if no piece found
+        next unless self[square]
+
+        #otherwise see if validly attacked
+        if self[square].side != side and self[square].role == :knight
+          #we found an attacker
+          return true
+        end
+
+    end
+    false
+  end
 
   #These variables help us track what's happened and allow us to undo
   attr_accessor :piece_last_moved, :piece_last_moved_from_coord

trunk/app/models/piece.rb

@@ -36,10 +36,10 @@ class Piece
   attr_reader :lines_of_attack
   attr_reader :direct_moves
 
-  # A unique one of white's pieces, for instance
+  # A unique one of white's pieces, for instance. To differentiate pawns from each other
+  # The which parameter is appended, but this is not needed for unique pieces on a side like king
   def side_id
-    if ROLES_WITH_MANY.include?(@role) || @which
-      raise AmbiguousPieceError unless @which
+    if @which
       "#{@which}_#{@role}".to_sym 
     else
       "#{@role}".to_sym
@@ -48,7 +48,6 @@ def side_id
 
   # A unique piece across the whole board
   def board_id
-    raise AmbiguousPieceError unless @side
     "#{@side}_#{side_id}".to_sym
   end      
 
@@ -145,5 +144,3 @@ def abbrev_to_role(char)
 
 end
 
-class AmbiguousPieceError < Exception
-end

trunk/spec/models/board_spec.rb

@@ -94,21 +94,20 @@
 
   #TODO: the overall time to detect checkmate (in actual checkmate situation) has gotten much worse (5x) since adding checkmate detection at the end of every move.
   # 2 notes: 1) commenting out the after_save callback in move makes things 400% better
-  # 2) there seems to be a problem checking for in_check? inside consider_move - some pieces
-  # seem not to be found underneath their keys leading to a NoMethodError calling nil.side
-  #it 'should be able to detect check (within a reasonable amount of time)' do
-  #  elapsed = Benchmark.realtime do
-  #    match = matches(:scholars_mate)
-  #    (1..50).each do 
-  #        #match.moves << Move.new( :from_coord => :c4, :to_coord => :f7 )  #bishop, not a mate
-  #        match.moves << Move.new( :from_coord => :h5, :to_coord => :f7 )  #queen , a mate
-  #        #test for checkmate
-  #        board = match.board
-  #        board.in_check?(:black).should be_true
-  #        match.moves.last.destroy
-  #    end
-  #  end
-  #  puts "50 checks tested in #{elapsed} seconds (on the PC each check for check takes .1 sec on average)"
-  #end
-
+=begin  
+  it 'should be able to detect check (within a reasonable amount of time)' do
+    elapsed = Benchmark.realtime do
+      match = matches(:scholars_mate)
+      (1..50).each do 
+          #match.moves << Move.new( :from_coord => :c4, :to_coord => :f7 )  #bishop, not a mate
+          match.moves << Move.new( :from_coord => :h5, :to_coord => :f7 )  #queen , a mate
+          #test for checkmate
+          board = match.board
+          board.in_checkmate?(:black).should be_true
+          match.moves.last.destroy
+      end
+    end
+    puts "50 checks tested in #{elapsed} seconds (on the PC each check for check takes .1 sec on average)"
+  end
+=end
 end

trunk/spec/models/piece_spec.rb

@@ -23,38 +23,22 @@
     @black_queen.role.should == :queen
   end
 
-  it 'may be a bishop if you specify which one' do
-    @black_queens_bishop.should_not be_nil
+  it 'may be a bishop' do
+    @black_queens_bishop.kind_of?(Piece).should be_true
   end
 
-  it 'may not be a bishop if you do not specify which one' do
-    lambda{ bishop = Bishop.new(:white) }.should raise_error
+  it 'may be a knight' do
+    @white_kings_knight.kind_of?(Piece).should be_true
   end
 
-  it 'may be a knight if you specify which one' do
-    @white_kings_knight.should_not be_nil
+  it 'may be a rook' do
+    @white_kings_rook.kind_of?(Piece).should be_true
   end
 
-  it 'may not be a knight if you do not specify which one' do
-    lambda{ knight = Knight.new(:white) }.should raise_error
+  it 'may be a pawn' do
+    @d_pawn.kind_of?(Piece).should be_true
   end
 
-  it 'may be a rook if you specify which one' do
-    @white_kings_rook.should_not be_nil
-  end
-
-  it 'may not be a knight if you do not specify which one' do
-    lambda{ rook = Rook.new(:white) }.should raise_error
-  end
-
-  it 'may be a pawn if you specify which one' do
-    @d_pawn.should_not be_nil
-  end
-
-  it 'may not be a pawn if you do not specify which one' do
-    lambda{ pawn = Pawn.new(:white) }.should raise_error
-  end
-
   it 'should tell you its desired moves (what it could do on an empty board) from a given position' do
     #desired_moves_from is a common interface to all pieces
     moves = Pawn.new(:white, :d).desired_moves_from( :d3 )