Queen Attack: Replace tuple with ChessPosition #119
Conversation
| fn position(&self) -> &ChessPosition; | ||
| fn can_attack<T: ChessPiece>(&self, other: &T) -> bool; | ||
| } | ||
| // |
There was a problem hiding this comment.
do you mean for this comment to be here?
|
Yeah seems like a good way to address what's discussed in #118. Tell me about the changing of the |
| let white_queen = Queen::new((2,2)).unwrap(); | ||
| let black_queen = Queen::new((0,4)).unwrap(); | ||
| assert!(white_queen.can_attack(&black_queen)); | ||
| fn queens_on_the_same_diagonal_can_attack_one() { |
There was a problem hiding this comment.
Hmm, while we are changing the test names, maybe we can take the opportunity to bikeshed. I could see these being upper_right, upper_left, etc. rather than just one, two, etc.
But I don't actually care (I recognise I'm bikeshedding), so you can pick whatever makes the most sense.
There was a problem hiding this comment.
Yeah, I couldn't think of a good name for them either.
|
|
|
Okay I say 👍 with squash (I think you'll agree on the squash) Would like to hear what others involved think, of course. |
Fixes exercism#118 `Queen::new` now expects a `ChessPosition` instead of a tuple. `ChessPosition::new` returns a Result, indicating if the position is valid or not. I've added a few tests at the start to capture the behavior of `ChessPosition`. The tests of Queen have their API usage updated, and have been renamed for readability.
IanWhitney
force-pushed
the
replace_queen_attack_tuple_with_position
branch
from
10e349b to
275919a
Compare
Fixes #118
Queen::newnow expects aChessPositioninstead of a tuple.ChessPosition::newreturns a Result, indicating if the position is valid or not.I've added a few tests at the start to capture the behavior of
ChessPosition. The tests of Queen have their API usage updated, and have been renamed for readability.