[#62] [WIP] En Passant Logic For Pawn

[Se7enB2st]

-Dependent on Turn Base Logic PR

This is building off Chad's obstruction PR.

Add:

• first_turn! to game model
• end_turn! to game model
• En Passant logics to pawn model
• move_number to game database table
• en_passant to game database table
• last_moved_piece to piece database table

Not including:

• Pawn Capture Logic
• En Passant Capture Logic (Dependent on Pawn Capture Logic)

En Passant Logic:

1. Target pawn just moved 2 space forward
2. Target pawn only capturable within the next turn
3. Target pawn can only be captured by pawn of opposite color
4. Opposite Pawn must be adjacent to the Target pawn ( x+1 or x-1)

There is probably some refactoring can be done, feel free to leave feedbacks.

[lasley]

Rebased onto master for branch isolation

[chadbaum]

I suggest we merge pawn capture logic first so Ethan can then complete en passant logic as noted in desc.

[chadbaum]

Now that pawn capture is in, pls confirm work is done and PR is ready for review by removing WIP tag and commenting.

[Se7enB2st]

Integrated En Passant Logic into Pawn's valid_move? logic. En Passant Logic now works with piece move! logic.

[lasley]

Looks like we have conflicts

[Se7enB2st]

Fixed Merge Conflicts, also add a test to make sure En Passant Logic only works on enemy pawn.

[chadbaum]

Can you explain your schema decisions? Scanning your code, you have added:

Piece: last_moved_piece
Game: en_passant, move_number, turn,

As far as I can tell:

• En_passant is not used for anything in your code, please delete or explain.
• As discussed at last 2 meetings, move_number and turn are both redundant, you only need one. An odd turn means it is white player's turn, an even turn means it is black player's turn. There is no need for another column here. I would delete move_number and just make turn an incrementing integer.
• I would rename last_moved_piece as it is both redundant (don't need to say piece when talking about piece model) but also vague...I think what you meant is something like turn_last_moved.

This also takes care of the turn logic in the next ticket so I'm not sure there is any work to be done there. When this is done we can surface what turn it is in the games#show view with a simple `<%= game.turn %>'.

Also, I noticed a bunch of surface-level changes to files unrelated to en passant, do you know what caused them and if we should be reviewing any of it? If you look at your files changed, you have many initializers, application.rb, and environment files included. Want to make sure you're not wasting your time Rubocopping any of these files -- they should have already been blacklisted.

[Se7enB2st]

En Passant is not needed at this point(It was necessary when I first attempt at En Passant Logic), I will clean up the database to remove/change any redundant columns.

The surface-level changes were due to Rubocop errors. It kept asking me to add top-level comments at those files(Not sure why it was doing this if it was blacklisted). But there is no other change made to those files beside the top comments.

[chadbaum]

@aleksgorbenko Do you know why Ethan running Rubocop would have caused violations for these files when it doesn't for the rest of us?

@Se7enB2st I wouldn't waste your time Rubocopping Rails files. I've not had any problems with the whitelist/blacklist since Aleks implemented it so maybe he can investigate.

[aleksgorbenko]

i think for readability it should be:

return true if adjacent_piece.type == 'Pawn' &&
               adjacent_piece.color != color

[Se7enB2st]

Rubocop approve

[chadbaum]

I agree with Aleks, this should be refactored as he suggested for best formatting. But it is minor.

[aleksgorbenko]

@chadbaum re: Rubocop - no idea really. I just checked the rubocop.yml file and it is in place in this branch. Not sure what could have caused that.

[chadbaum]

I'm not sure what step 3 means. En Passant does not require that the ONLY piece available to capture the victim be the pawn, only that the pawn COULD have captured it in the first space on the previous turn.

[aleksgorbenko]

I can't say I understand the methods entirely - it would be best to discuss on a call.

[chadbaum]

Don't need this guard clause.

[Se7enB2st]

Tried to remove this line, but it causes the method to return Nil class error.

[chadbaum]

Can you explain what the adjacent parameter is doing? I see you passing true and false between these 2 methods but they are not commented.

[chadbaum]

Just because a pawn is adjacent to another pawn does not make it a valid en passant victim so I think the name and return of this method are misleading. What makes it valid is this PLUS the victim having moved 2 spaces forward last turn.

[Se7enB2st]

This method checks if there is any valid pawn adjacent to the victim pawn(include? true or false). If any of adjacent side have a valid pawn, it will return true. A separate method check if the pawn moved 2 space forward in the last turn.

This was the rafactored version of following methods:

def valid_en_passant_pawn?(x, y)
return false unless valid_passant_pawn_left?(x, y)
return false unless valid_passant_pawn_right?(x. y)
false
end

def valid_passant_pawn_left?(x, y)
left_piece = find_piece(x - 1, y)
return false if left_piece.nil?
return true if left_piece.type == 'Pawn' && side_piece.color != color
false
end

def valid_passant_pawn_right?(x, y)
right_piece = find_piece(x + 1, y)
return false if right_piece.nil?
return true if right_piece.type == 'Pawn' && side_piece.color != color
false
end

[lasley]

I don't see the other method you are referring to that checks the 2 spaces forward. IMO this logic should all be one method, it is really hard to follow otherwise.

[lasley]

This is good to review right, or are you guys still wokring on it?

[Se7enB2st]

Yes, I will refactor more once turn base logic is merged.

[chadbaum]

This still does not seem ready to merge as there are many pending questions, so we will fix this for the next version release.