darts: augment test suite#1448
Conversation
ErikSchierboom
left a comment
ErikSchierboom
left a comment
There was a problem hiding this comment.
Adding those additional tests for negative cases seems like a good addition! I'm wondering a little bit about the number of test cases, and if it perhaps were possible to be less exhaustive but still do the same checks (negative/non-negative, border/center, etc.). Do you think that would be possible?
|
The tests already combine coordinate signedness and target area/shape into one. There is a test for well away from the target, outside but near the target, and then, roughly, for each ring, it tests a hit on the very border, in the middle of the ring at a zero-coordinate, in middle of the ring at a 45° angle, and in the ring but near the border to another ring at a 45° angle (this is where tests would fail if the target was treated as a square instead of a circle in a solution). The coordinates "rotate" around at the same time, starting at negative x and positive y, to positive x and positive y, to positive x and negative y, and so on. I'm not sure which to drop. The way-outdside-the-target test could go. Other obvious candidates are those for the middle of the ring at a zero-coordinate, they serve no definite purpose, but there are only 2 of those. Next could be those at the middle of a ring at a 45° angle, there are 2 of those as well. The tests for hits at the very border should stay, I would say, as should the tests for hits near the border to another ring. And the center hit should be there as well, since where is the fun in a dart match if you don't have one of those ;). |
|
The way I perhaps would go about this is to modify some of the existing test cases to use negative values. E.g. one you use |
|
Ok, can do. But what actually prompted me to make this PR was that the existing test cases allowed for the target to be treated as a square (instead of a circle, as a dart target normally is, and is also mentioned in the instructions), and still pass. So, what I would do now, to keep the number of test cases small, is use the existing test cases with different coordinates, namely negative/positive mixed at points that will pass if the target is treated as a circle but fail if treated as a square. Would that be ok? |
To me, that is a great solution. I'd be curious to hear what @rpottsoh or @petertseng think about the best approach? |
|
Alright.... I agree that negative values for X and Y be introduced. I think of a dart board more as a Polar system more than a Cartesian system. Since this is a mathy exercise I got to thinking that maybe it would be more interesting if the coordinates were Polar... There would not be any negative values.... A quick search lead me here. A nice explanation of converting from Cartesian to Polar and back.... Since we are thinking of a Cartesian system then we obviously should be considering the possibility of X and/or Y being negative as the coordinates locate a thrown dart having landed in one of the 4 quadrants of the Cartesian system..... I am not quite sure at this point if it is better to add tests, alter existing tests, or both.... Perhaps as long as there is at least one case for each of the 4 quadrants would be a good enough change? |
I agree! @juhlig do you think that is do-able? |
|
It is doable, yes, but I don't fully agree. There should at least be tests for hits at the very borders between the rings (border hit counts as a hit to the more inward ring), as this ensures the proper usage of |
Alright. Perhaps you can add those as new test cases then? |
|
Ok, done. There are 7 tests now, one for each quadrant, one for each border. |
| "x": 15.3, | ||
| "y": 13.2 | ||
| "x": -9, | ||
| "y": 9 |
There was a problem hiding this comment.
This almost had me thinking it was inside the target. I calculate the distance from the center to be ~12.7 so checks out for this case. 👍
rpottsoh
left a comment
rpottsoh
left a comment
There was a problem hiding this comment.
The changes all look good and check out for me. 👍 If there are not further objections/requests for changes then this should be merged in the next 24-48 hours.
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
the treatment of darts landing on borders doesn't seem consistent, based on what I have noted. I request an explanation of the rules, or a change to be consistent
| "x": 5, | ||
| "y": 0 | ||
| }, | ||
| "expected": 5 |
There was a problem hiding this comment.
note that here, the dart is treated as being in the smaller circle (here, the middle one) rather than the larger one (outer)
| "y": 5 | ||
| "y": -1 | ||
| }, | ||
| "expected": 5 |
There was a problem hiding this comment.
here, the dart is treated as being in the larger circle (middle) rather than the smaller (inner)
There was a problem hiding this comment.
You are right, this should count as a hit to the inner circle. Fixed in my last commit.
@juhlig @petertseng raises some valid points. Could you perhaps look at his comments? |
The test suite does not include tests that ensure the target is treated as a circle (as opposed to a square), and does not cover the left and lower quadrants (where x and/or y would be negative).
|
@petertseng Could you see if your comments have been addressed? |
|
I already ghave. |
|
Ah sorry, I didn't see your name in the list of approvers. |
|
it is a little too easy to miss it, since as of about 1-2 days ago dismissals still do not send a notification |
|
Merged. Thanks @juhlig! |
4 participants
I was just mentoring an exercise where the students solution treated the target as a square (as opposed to a circle), and was surprised that it nevertheless passed the tests.
I found that they only (except for one, that accidentially works still) use coordinates where one of them is
0, which also works if the solution calculates scores as if the target was a square.I also noticed that, while the target is supposed to be a circle centered at
(0, 0), so that there are valid target areas with negative coordinates, there are no test cases that provide negative coordinates at all.