Skip to content

darts: add tests to disallow absolute addition#1488

Merged
ErikSchierboom merged 2 commits intoexercism:masterfrom
yawpitch:patch-1
Mar 29, 2019
Merged

darts: add tests to disallow absolute addition#1488
ErikSchierboom merged 2 commits intoexercism:masterfrom
yawpitch:patch-1

Conversation

@yawpitch
Copy link
Copy Markdown
Contributor

The current tests allow an erroneous-but-passing solution where the sum of the absolute values of the coordinates of the dart are compared to the radius thresholds (ie return 5 if abs(x) + abs(y) is greater than 1 but less than or equal to 5), rather than the calculated distance of the dart from the origin. Thanks to grwkremilek for the find.

The current tests allow an erroneous-but-passing solution where the sum of the absolute values of the coordinates of the dart are compared to the radius thresholds (ie return 5 if `abs(x) + abs(y)` is greater than 1 but less than or equal to 5), rather than the calculated distance of the dart from the origin. Thanks to grwkremilek for the find.
@rpottsoh
Copy link
Copy Markdown
Member

Update versioning reminder.

@rpottsoh
Copy link
Copy Markdown
Member

rpottsoh commented Mar 26, 2019

The square root of the sum of the squares of x and y yields the correct distance from origin of the dart board. Thanks for adding these tests I think they will certainly help catch solutions that are not calculating the distance correctly.

@petertseng
Copy link
Copy Markdown
Member

Since it came up today.

My approval is conditional on commits being squashed. (It doesn't matter to me whether the submitter squashes them or the merger squashes them, as long as someone does)

@yawpitch
Copy link
Copy Markdown
Contributor Author

yawpitch commented Mar 29, 2019 via email

@ErikSchierboom ErikSchierboom merged commit bac9381 into exercism:master Mar 29, 2019
@ErikSchierboom
Copy link
Copy Markdown
Member

Squashed and merged. Thanks @yawpitch!

@rpottsoh
Copy link
Copy Markdown
Member

Thinking more about this change.... I still agree it was a good change, but am now wondering would it have been better to just update

  • "A dart lands in the outer circle" with x = 4 and y = 8,
  • "A dart lands in the middle circle" with x = 2 and y = 4, and
  • "A dart lands in the inner circle" with x = 0.4 and y = 0.8.

It would then not be necessary to add three more tests.

@yawpitch
Copy link
Copy Markdown
Contributor Author

I had considered doing that, but the problem is that someone could revert that change or otherwise modify those values in the future and inadvertently and silently put the tests back into a place where they have a gap in coverage for certain values of x and y. The only mechanism for flagging that the specific chosen points are significant -- as opposed to them being arbitrary points that fall within the chosen rings -- is the "description" field, which isn't exactly robust, so by making them distinct tests they both close the gap and make it harder to inadvertently re-open it, hopefully without introducing any significant overhead.

@rpottsoh
Copy link
Copy Markdown
Member

Excellent point. Thanks for clarifying.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants