Implement Queen Attack #89

IanWhitney · 2016-03-26T17:29:23Z

There is no common set of tests for this exercise, so implementation is
a bit....free form. I think there are two interesting things to look at,
what tests to use, and how to use them.

Which Tests

I first surveyed all of the languages that implement this exercise and
looked at what they tested. In order of prevalence, the five most common
test types cover:

Can 2 queens attack each other?
Can 2 pieces occupy the same
space?
Comparing string representations of the
chessboard
Does a chessboard start with the queens in their default
positions?
Errors are raised if an invalid position is
used

Then there are some uncommon edge-case tests. Can there be more than one
chessboard? Can the board be empty? Algebraic chess notation? Etc.

Of the top five, I found that only numbers 1 and 5 really cover the
exercise as stated -- if I place two pieces on the board, can they attack
each other?

So, those are the tests that I've implemented here.

Implementing the other tests requires the introduction of a Chessboard
object. And also a way to identify pieces (which team they belong to,
how they are represented as strings, etc.) That can be interesting to
do, but I find it beyond the scope of this exercise as written.

At best, I think these tests should be suggested as Extra Credit.

What Order

I was very particular in my test order. Some languages, for example,
start with the board representation tests. This requires students to
write a lot of code in order to get just one test to pass.

I have found that the faster I can get the first test to pass, the
better. So my first test requires nothing more than a can_attack
function that returns false. The tests then build on that, adding
complexity. Then, once can_attack is fully implemented, I add the
curveball test of invalid chess positions, which will get students
thinking about bad inputs and Rust's error handling.

IanWhitney · 2016-03-26T17:30:03Z

If you're interested in the tests implemented by other languages, I've summarized it here: https://gist.github.com/IanWhitney/dd34f8a16c730f3cd77c

IanWhitney · 2016-03-26T21:06:37Z

@petertseng, it sounds like some of the survey I did for this problem overlaps well with your recent work (as outlined in today's newsletter). So if I can contribute to a canonical version of the Queen Attack tests, let me know. Happy to help.

petertseng · 2016-03-26T21:55:41Z

Many thanks for conducting the survey @IanWhitney !!

Since I will be traveling, remind me if I haven't gotten back to this PR in 72 hours from now. Of course anyone else is free to beat me to the punch on this!

In the meantime, my remarks on the cross-track consistency work:

A great first step will be to open a PR in https://github.com/exercism/x-common adding queen-attack.json with what you think are the tests that are in fact relevant to the exercise, as you described in your commit message/PR description. It will be good if your PR references https://github.com/exercism/todo/issues/133. In that PR will probably discuss with @kytrinyx and any interested track maintainers to get agreement on the set of tests.

After that is merged, then the next thing is to get tracks in line with it, adding or removing tests.

We have a tool https://github.com/exercism/blazon for filing issues across all tracks with a given problem, we can help draft an issue text if you like.

Of course you are free to file PRs actually making the changes for some (or even all?!) tracks if you desire, but it's totally fine to load-balance across multiple contributors.

All of the above are absolutely open for you to take charge of currently, since I should probably only focus on one problem at a time and my current efforts are focused on a different problem. So it's yours if you want it!

petertseng · 2016-03-26T22:06:48Z

exercises/queen-attack/example.rs

+}
+
+impl Queens {
+    pub fn new(first: Result<Queen, InvalidQueen>, second: Result<Queen, InvalidQueen>) -> Queens {


I'm thinking a bit about the interface here; seems a bit odd to have those Result<Queen, InvalidQueen> as the argument types.

What if it were (i8, i8) here instead of the Result<Queen, InvalidQueen>, and returning Result<Queens, InvalidQueen> (or maybe InvalidQueens? Or just Option<Queens>?)

I can understand why the Result<Queen, InvalidQueen> is done (so that the result of Queen::new can be directly passed in, and then Queen::new can be tested individually instead of Queens::new which has more decision-making to do). Just seems an unclear interface.

What if it were just queen1.can_attack(queen2) ?

I could be convinced either way on this, feel free to discuss it while I'm traveling.

I'm totally open to changing the interface. If I were writing this from scratch in Ruby I probably would do queen1.can_attack?(queen2).

I think my API was mostly being led by the other implementations of this exercise, which tend to look like

queens = Queens.new(white: [2, 3], black: [4, 7]) assert !queens.attack?

But since I'm trying to re-think which tests are appropriate, I might as well also re-think what API is appropriate. Let me take a shot at the queen1.can_attack(queen2) approach.

https://github.com/exercism/todo/issues/133 While implementing Queen Attack for Rust (exercism/rust#89), I surveyed all current implementations of this exercise and documented which tests they used. That full list of tests is here: https://gist.github.com/IanWhitney/dd34f8a16c730f3cd77c In order of prevalence, the five most common test types cover: 1. Can 2 queens attack each other? 2. [Can 2 pieces occupy the same space](https://github.com/exercism/xelixir/blob/0965c4e107de68979a493b82f9f0859773fcb83b/exercises/queen-attack/queen_attack_test.exs#L26)? 3. [Comparing string representations of the chessboard](https://github.com/exercism/xelixir/blob/0965c4e107de68979a493b82f9f0859773fcb83b/exercises/queen-attack/queen_attack_test.exs#L33) 4. [Does a chessboard start with the queens in their default positions?](https://github.com/exercism/xelixir/blob/0965c4e107de68979a493b82f9f0859773fcb83b/exercises/queen-attack/queen_attack_test.exs#L12) 5. [Errors are raised if an invalid position is used](https://github.com/exercism/xpython/blob/b9dfe291845a31301029d947b3cf3a2ffc85b0b6/exercises/queen-attack/queen_attack_test.py#L55) Then there are some uncommon edge-case tests. Can there be more than one chessboard? Can the board be empty? Algebraic chess notation? Etc. Of the top five, I found that only numbers 1 and 5 really cover the exercise as stated -- if I place two queens on the board, can they attack each other? So, those are the tests that I've documented in this json file. Implementing the other tests requires the introduction of a Chessboard object, and also a way to identify pieces (which team they belong to, how they are represented as strings, etc.) That can be interesting to do, but I find it beyond the scope of this exercise as written. At best, I think these tests should be suggested as Extra Credit. Test order also varied between implementations. I have found that the faster I can get the first test to pass, the better. So my first test requires nothing more than a `can_attack` function that returns false. The tests then build on that, adding complexity. Then, once `can_attack` is fully implemented, I add the curveball test of invalid chess positions, which will get students thinking about bad inputs and error handling.

There is no common set of tests for this exercise, so implementation is a bit....free form. I think there are two interesting things to look at, what tests to use, and how to use them. Which Tests I first surveyed all of the languages that implement this exercise and looked at what they tested. In order of prevalence, the five most common test types cover: 1. Can 2 queens attack each other? 2. [Can 2 pieces occupy the same space](https://github.com/exercism/xelixir/blob/0965c4e107de68979a493b82f9f0859773fcb83b/exercises/queen-attack/queen_attack_test.exs#L26)? 3. [Comparing string representations of the chessboard](https://github.com/exercism/xelixir/blob/0965c4e107de68979a493b82f9f0859773fcb83b/exercises/queen-attack/queen_attack_test.exs#L33) 4. [Does a chessboard start with the queens in their default positions?](https://github.com/exercism/xelixir/blob/0965c4e107de68979a493b82f9f0859773fcb83b/exercises/queen-attack/queen_attack_test.exs#L12) 5. [Errors are raised if an invalid position is used](https://github.com/exercism/xpython/blob/b9dfe291845a31301029d947b3cf3a2ffc85b0b6/exercises/queen-attack/queen_attack_test.py#L55) Then there are some uncommon edge-case tests. Can there be more than one chessboard? Can the board be empty? Algebraic chess notation? Etc. Of the top five, I found that only numbers 1 and 5 really cover the exercise as stated -- if I place two pieces on the board, can they attack each other? So, those are the tests that I've implemented here. Implementing the other tests requires the introduction of a Chessboard object. And also a way to identify pieces (which team they belong to, how they are represented as strings, etc.) That can be interesting to do, but I find it beyond the scope of this exercise as written. At best, I think these tests should be suggested as Extra Credit. What Order I was very particular in my test order. Some languages, for example, start with the board representation tests. This requires students to write a lot of code in order to get just one test to pass. I have found that the faster I can get the first test to pass, the better. So my first test requires nothing more than a `can_attack` function that returns false. The tests then build on that, adding complexity. Then, once `can_attack` is fully implemented, I add the curveball test of invalid chess positions, which will get students thinking about bad inputs and Rust's error handling.

As discussed here: http://doc.rust-lang.org/std/result/enum.Result.html#variant.Err I like the `queen.can_attack(queen2)` API a lot better. It has a downside of `Queen::new` returning a Result, so all tests that check for valid attacks have to call `unwrap` Though that might just be because there's a better way to do this that I am unaware of. I'm still quite new to Rust, especially the error-handling part of it. The us of `is_err` was recommended by Manishearth in this thread https://users.rust-lang.org/t/what-am-i-not-understanding-about-result/5143/11?u=ianwhitney

kytrinyx · 2016-03-27T23:04:45Z

FYI @petertseng I merged the PR in x-common: exercism/problem-specifications#211

petertseng · 2016-03-28T07:56:07Z

Yeah, I guess either can_attack or Queen::new has to deal with the possibility of an invalid queen.

What would you say to having that be in Queen::new instead? So Queen::new could return a Option<Queen> or maybe a Result<Queen, Something> (I think a error type of () isn't all that useful and one might as well use Option in that case, but if there is some sensible errror type we coud consider it).

I realize that yes, you'd still have to unwrap() the result of that Queen::new in the tests, so it's not like it's saving us any unwrapping. But it seems useful to me to raise errors as close to the site of the error as possible.

Another reason for that would be that then it's obvious what queen caused the problem. If I have queenA.can_attack(queenB) which is an Error, how do I know what queen caused the problem? I could have a known-good Queen to call against both queens and see which one errors, but that seems cumbersome. Seems easier for clients of the code if we can know at construction time that we will either get a valid Queen, or an error right then.

(By the way the commit message of 2a74be0 implies that Queen::new is returning the Result like I suggest, but in fact it is can_attack that has the Result)

This is just what I think but I could be convinced otherwise, totally open to hearing what @IanWhitney or @EduardoBautista (or anyone else!) have to say about that.

IanWhitney · 2016-03-28T16:06:45Z

Doing it on Queen::new does seem more in line with the common test suite

But if I do it that way, then every test would have lines like:

  let white_queen = Queen::new((2,4)).unwrap();

Is there a way in Rust to have students avoid using a Result value until they reach the test that introduces error handling? Because that's what I think would be best in terms of learning curve. I'd love students' initial implementations to be simple,

pub fn new(position: (i8,i8)) -> Queen {
  Queen {position: position}
}

Because the simple implementation gets tests passing faster, which I always think is a good thing.

And then when the student hits the error handling test they have to figure out how to modify their simple implementation to handle this extra complexity.

IanWhitney · 2016-03-28T16:07:53Z

Oh, and my example is returning an empty-tuple error because I didn't see a good reason for it to return anything more complex. Students could return any error type they like. It just has to properly handle is_err().

petertseng · 2016-03-28T18:08:53Z

But if I do it that way, then every test would have lines like:
let white_queen = Queen::new((2,4)).unwrap();

Hmm, is that better or worse than there being lines of the form:

assert!(white_queen.can_attack(black_queen).unwrap());

which every test already does have?

One of the two has to have that unwrap if we are to have error handling at all.

Is there a way in Rust to have students avoid using a Result value until they reach the test that introduces error handling? Because that's what I think would be best in terms of learning curve. I'd love students' initial implementations to be simple,

This is a good thought.

We'd have to figure out how to get this accepted by the type checker if Queen::new should start out returning just a Queen but later should change to returning a Result<Queen, _>. It seems to imply that the earlier tests expecting a Queen would have to be deleted. Or at least not compiled, using some sort of conditional compilation thing? You could explore the conditional compilation route if you think it will be better. If it works out we may like to think about how other problems in this track can make use of it (separately from this PR of course).

Alternatively, is there a way that the test could provide the implementation of some_queen.unwrap() (which would presumably just be the identity function)? That way Queen::new((2,4)).unwrap() will work both when Queen::new is expected to return a Queen and when it's expected to return a Result<Queen, _>

If it turns out the above suggestions are working too hard against the type system, maybe it's a sign that in Rust, the type system means you have to check errors earlier than you might have to in other languages. Although I realize that statement is more about when you would check errors when you're using a library that provides a fixed interface. The question we're dealing with here is "when should we add error handling when iteratively developing, and when we add error handling, how are our tests affected?"

Oh, and my example is returning an empty-tuple error because I didn't see a good reason for it to return anything more complex. Students could return any error type they like.

Ah I see! I missed the fact that it just needs to be an error of some kind instead of being forced to be (). Great!

IanWhitney · 2016-03-28T19:22:55Z

Thinking about it more, I may be trying to implement Null Object Pattern in Rust. And you're right, that may be an idea that is incompatible with how Rust wants me to work. The pattern exists for OO languages which can return nulls, and Rust can't do that.

Maybe the best approach is to have Queen::new return a Result. And then I'll re-order the tests a bit to make initial progress easier.

@petertseng

Prompted by the discussion with @petertseng here: exercism#89 (comment) The standard tests show that Queen creation should throw an error, not the `can_attack` method. To follow that, I've switched `Queen::new` to return a `Result`. This has required a change to the tests, as each `Queen::new` now has to be unwrapped. I've reordered the tests to make that less weird (hopefully). The first test now exercises the creation of a Queen with a valid position, using `is_ok()` which should points students directly at the Result return type. We then test the invalid queen creation. Then we can move on to `can_attack`. And I've updated the Example implementation to match this new API, while also polishing it a bit.

IanWhitney · 2016-03-28T20:06:24Z

Ok. I think we're pretty good. Or at least I hope so, since I won't have nearly as much free time to hack on this stuff after today.

petertseng · 2016-03-28T22:30:05Z

I think this is pretty good as well! Given that Queen::new is returning the Result now rather than can_attack I think it definitely made sense to reorder the tests in the fashion given.

If it turns out that students are not figuring out that the return value should be of a Result type we can point it out to them via a comment in the test file or something. May want to periodically examine submissions to queen-attack to see whether anyone has such trouble.

Given I'm happy with this and nobody else had objections over the weekend let's get this merged. Thanks again for the work.

petertseng reviewed Mar 26, 2016
View reviewed changes

IanWhitney mentioned this pull request Mar 27, 2016

Add standard tests for Queen Attack exercism/problem-specifications#211

Merged

IanWhitney force-pushed the implement_queen_attack branch from ae94fe5 to 9e00097 Compare March 27, 2016 16:25

petertseng merged commit 62f8bbd into exercism:master Mar 28, 2016

IanWhitney deleted the implement_queen_attack branch March 30, 2016 01:51

petertseng mentioned this pull request Apr 29, 2016

Passing a tuple to new in Queen attack exercise feels weird #118

Closed

petertseng mentioned this pull request Oct 3, 2016

Implement Bowling #213

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Queen Attack #89

Implement Queen Attack #89

IanWhitney commented Mar 26, 2016

IanWhitney commented Mar 26, 2016

IanWhitney commented Mar 26, 2016

petertseng commented Mar 26, 2016

petertseng Mar 26, 2016

IanWhitney Mar 27, 2016

kytrinyx commented Mar 27, 2016

petertseng commented Mar 28, 2016

IanWhitney commented Mar 28, 2016

IanWhitney commented Mar 28, 2016

petertseng commented Mar 28, 2016

IanWhitney commented Mar 28, 2016

IanWhitney commented Mar 28, 2016

petertseng commented Mar 28, 2016

Implement Queen Attack #89

Implement Queen Attack #89

Conversation

IanWhitney commented Mar 26, 2016

IanWhitney commented Mar 26, 2016

IanWhitney commented Mar 26, 2016

petertseng commented Mar 26, 2016

petertseng Mar 26, 2016

Choose a reason for hiding this comment

IanWhitney Mar 27, 2016

Choose a reason for hiding this comment

kytrinyx commented Mar 27, 2016

petertseng commented Mar 28, 2016

IanWhitney commented Mar 28, 2016

IanWhitney commented Mar 28, 2016

petertseng commented Mar 28, 2016

IanWhitney commented Mar 28, 2016

IanWhitney commented Mar 28, 2016

petertseng commented Mar 28, 2016