Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

darts: Add new exercise #1363

Merged
merged 10 commits into from
Oct 23, 2018
Merged

darts: Add new exercise #1363

merged 10 commits into from
Oct 23, 2018

Conversation

ramadis
Copy link
Contributor

@ramadis ramadis commented Oct 9, 2018

New easy programming exercise, solved by using euclidean distance.

"version": "1.0.0",
"cases": [
{
"description": "Retun the correct amount earnt by a dart landing in a given point in the target problem.",
Copy link
Member

@rpottsoh rpottsoh Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While not wrong, earnt is not the common spelling, earned is the more common spelling.

"cases": [
{
"property": "result",
"description": "A dart lands outside the targetOnly a single book",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Period and space needed between target and Only?

"description": "A dart lands outside the targetOnly a single book",
"input": {
"x":15.3,
"y":13.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have at least one space following :. Some places a space is present and in others it is absent.

[Darts](https://en.wikipedia.org/wiki/Darts) is a game where players
throw darts to a [target](https://en.wikipedia.org/wiki/Darts#/media/File:Darts_in_a_dartboard.jpg).

In our particular instance of the game, the target rewards with 4 different amount of points, depending on where the dart lands:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think amount should be plural.

@rpottsoh rpottsoh changed the title Add new 'target' exercise target: Add new exercise Oct 9, 2018
@rpottsoh
Copy link
Member

rpottsoh commented Oct 9, 2018

The description depicts darts being thrown. Why not call this exercise Darts?

@ramadis
Copy link
Contributor Author

ramadis commented Oct 9, 2018

Sounds good!

@ramadis ramadis changed the title target: Add new exercise darts: Add new exercise Oct 9, 2018
@rpottsoh
Copy link
Member

rpottsoh commented Oct 9, 2018

I should have opened with this instead of nit picking... Thanks for working on this. More exercises are always welcome! No linting errors! 🥇

* If the dart lands in the middle circle of the target, player earns 5 points.
* If the dart lands in the inner circle of the target, player earns 10 points.

The outer circle has a radius of 10 units (This is equivalent to the total radius for the entire target), the middle circle a radius of 5 units, and the inner circle a radius of 1. Of course, they are all centered to the same point (That is, the circles are [concentrics](http://mathworld.wolfram.com/ConcentricCircles.html)).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concentric should be singular.

@rpottsoh
Copy link
Member

rpottsoh commented Oct 9, 2018

It is my assumption that this exercise will eventually be merged. Being a new exercise an icon will need to be created. We (you and I) do not have to worry about creating this icon. This is the responsibility of the development team. However, they will need to be notified via an issue in exercism/website-icons. Refer to #1271 for the limited conversation that has taken place on the subject. This is a new activity that we must undertake when a new exercise comes our way and so the procedure is not practiced.

At some point an issue would need to be created and this PR should be referenced. The development team can then more conveniently monitor the status of this PR to know when it has been merged or if it was closed (to know if an icon is needed).

rpottsoh
rpottsoh previously approved these changes Oct 9, 2018
},
{
"property": "result",
"description": "Input is not a number",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this, expected should be an object containing one property, error, with its value being a string describing the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that any invalid input should return null. Maybe I should make it clear in the description, or go with the error. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention is that an error condition should be formatted as follows: "expected": {"error": "empty stack"}
Each track implementing this exercise can then for itself decide how to represent the error case.

Copy link
Member

@rpottsoh rpottsoh Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the case should be excluded from the exercise. See #902 and #523. Since erroneous input is not covered in description.md I see no reason to cover it in test data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ramadis What do you think about excluding this (error) case from the exercise? @rpottsoh has linked to two relevant issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ErikSchierboom Yes, I'm inclined to go that way. I believe also that adding tests for inputs that are outside the problem domain in the problem specification could bring inconsistencies between implementations. Since, for example, a language with a strong type system (e.g. Haskell) shouldn't have to test for these cases since the program would not compile given incorrect input, and making it throw an exception would be counterproductive to the simplicity and core value of the problem.

So, I'll remove this test and keep going on!

@ramadis
Copy link
Contributor Author

ramadis commented Oct 9, 2018

I've just opened the issue for the logo creation: exercism/website-icons#13

},
{
"property": "result",
"description": "Input is not a number",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention is that an error condition should be formatted as follows: "expected": {"error": "empty stack"}
Each track implementing this exercise can then for itself decide how to represent the error case.

"description": "Return the correct amount earned by a dart landing in a given point in the target problem.",
"cases": [
{
"property": "result",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps score would be a more descriptive propery name?

@rpottsoh rpottsoh dismissed their stale review October 10, 2018 15:30

considerations regarding incorrect input need to be discussed.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one tiny nit.

exercises/darts/canonical-data.json Outdated Show resolved Hide resolved
@rpottsoh
Copy link
Member

rpottsoh commented Oct 16, 2018

Minor nit pick, sorry. What is considered a Darts game? The opening sentence says to return the points earned in a Darts game. None of the tests seems to be more than a single toss. Perhaps a small rewording of the description is in order? If a single toss can constitute a game then OK. Otherwise clarification between description and canonical data is probably needed.

I am not going to revoke my approval at this point because I think this is a pretty small detail that will not likely really trip anyone up too much.

Copy link
Member

@rpottsoh rpottsoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks for rewording.

@rpottsoh
Copy link
Member

@sdublish do you have any more input to offer? I think otherwise this PR is about ready to be squashed and merged.

@rpottsoh
Copy link
Member

rpottsoh commented Oct 22, 2018

This PR is going on two weeks. It has two approvals. I will plan to squash and merge in the next 24 hours unless I see good reason to hold off.

@ramadis
Copy link
Contributor Author

ramadis commented Oct 22, 2018

@rpottsoh Are you squashing it or do you want me to squash the commits?

@rpottsoh
Copy link
Member

You don't have to. If anyone else comes along in the mean-time it would be better for them to find things in their current state, not squashed.

@sdublish
Copy link
Contributor

@sdublish do you have any more input to offer? I think otherwise this PR is about ready to be squashed and merged.

Sorry for not getting back to you earlier. I think it looks pretty good; one thing I would suggest is making it explicit in the description that the center of the darts board is at (0,0). While I think most people would assume that, it would still be helpful.

@petertseng
Copy link
Member

Before merging, please remove .DS_Store and exercises/.DS_Store, thank you.

@rpottsoh rpottsoh merged commit 2eb93f6 into exercism:master Oct 23, 2018
@rpottsoh
Copy link
Member

@ramadis congrats on adding a new exercise and thanks for contributing to Exercism! 🥇

@ErikSchierboom
Copy link
Member

Well done @ramadis! 🎉

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.

5 participants