Skip to content

Comments

Check new#28

Open
dannypoit wants to merge 18 commits intomasterfrom
check-new
Open

Check new#28
dannypoit wants to merge 18 commits intomasterfrom
check-new

Conversation

@dannypoit
Copy link
Contributor

This is a rebased version of the check PR, which includes the x and y axis fix.

@dannypoit dannypoit requested a review from MattW216 November 2, 2017 02:22
def check?
# code
white_king = King.find_by(color: 'white')
black_king = King.find_by(color: 'black')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is looking at all of the King's in our database.

We only want the Kings inside of this game.

I think you can do... pieces.kings.find_by... (try it in a console to see) 🎉

Copy link
Contributor Author

@dannypoit dannypoit Nov 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pieces.kings.find_by didn't seem to work, because it said kings was not a valid method.

However, I changed it to the following:
white_king = King.find_by(color: 'white', game: self)
black_king = King.find_by(color: 'black', game: self)

Please let me know if this is okay.

queens.each do |queen|
return true if queen.valid_move?(white_king.position_x, white_king.position_y) && queen.color != white_king.color
return true if queen.valid_move?(black_king.position_x, black_king.position_y) && queen.color != black_king.color
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice how each of these blocks are the same.

Could dry this up by doing.

pieces.where.not(type: 'King').each do |piece|
  ##...
end  

I'm not sure if this is valid Rails 5 code: pieces.where.not(type: 'King')

Give it a try in a console 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pieces.where.not(type: 'King') did seem to work. I updated this part of the check? method. Please let us know if this is right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubocop changed each to find_each in pieces.where.not(type: 'King').find_each do |piece|.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to find_each broke the check? method. I changed this back to each by doing the following, and it seemed to work:
non_king_pieces = pieces.where.not(type: 'King')
non_king_pieces.each do |piece|

it 'returns true if the king is in check by knight' do
game = Game.create
game.populate_board!
king = King.find_by(color: 'black')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this finds ALL kings in the database. We want only kings within this game.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added game: game into the arguments. Tested with pry, and this seemed to work. Please let us know if this looks good.

@dannypoit
Copy link
Contributor Author

All the RSpec tests for check? seem to be failing because the valid_move? methods are not yet written/merged for all the piece types. Since we refactored the check? method per your suggestions (where we consolidated all the non-king pieces into one block), what I think happens when each RSpec check? test is run is that it starts checking all the non-king pieces, starting with pawns. Because valid_move? doesn’t exist on the pawn model yet, it immediately throws an error that that method is undefined, then moves onto the next test. So all the tests are failing with this same error:

NoMethodError:
undefined method `valid_move?' for #Pawn:0x007fe29cfc67b0

@mscoutermarsh
Copy link
Contributor

cc @ddunbar88 - is #25 coming together? It's needed for check to be completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants