Skip to content

updated minesweeper canonical data to contain borders#1594

Closed
sprinkle24 wants to merge 3 commits intoexercism:masterfrom
sprinkle24:issue_1555_minesweeper_canonical_data
Closed

updated minesweeper canonical data to contain borders#1594
sprinkle24 wants to merge 3 commits intoexercism:masterfrom
sprinkle24:issue_1555_minesweeper_canonical_data

Conversation

@sprinkle24
Copy link
Copy Markdown

No description provided.

@sshine
Copy link
Copy Markdown
Contributor

sshine commented Oct 2, 2019

Interesting addition. Is there a prior discussion of this?

I think this falls under the category of improvements that are held back by #1560.

@sshine sshine added the hold label Oct 2, 2019
@sprinkle24
Copy link
Copy Markdown
Author

Ok, I found this issue: #1555
did not know it was blocked :).

Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

when merging, please amend the commit messages to:

  • Link to the issue #1555
  • Summarise the reason for the change in this commit message. A short summary such as "So that the description and the tests are consistent" will be acceptable.

Please also see two comments on the tests

Comment thread exercises/minesweeper/canonical-data.json Outdated
Comment thread exercises/minesweeper/canonical-data.json Outdated
Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

The problem-specifications reviewers should also decide: Should there be tests for an invalid border? Here are some possible outcomes I see:

  • Yes, but they should be in a different PR. An issue should be created.
  • Yes, and they must be in this PR before it is merged.
  • No, and this PR should change the description to state that the border will always be valid.
  • No, and no description change is necessary (AKA, tracks are able to add them if desired, but we won't place them in the file here).

I have no preference and will default to the last option if no other preferences are stated.

So that the description and the tests are consistent, I added borders to the minefields in canonical-data.json
@sprinkle24
Copy link
Copy Markdown
Author

when merging, please amend the commit messages to:

Please also see two comments on the tests

Done.

@sprinkle24 sprinkle24 closed this Oct 3, 2019
@sprinkle24 sprinkle24 reopened this Oct 3, 2019
@sprinkle24
Copy link
Copy Markdown
Author

Solved the two requested changes.

@petertseng petertseng requested review from petertseng and removed request for petertseng October 3, 2019 15:56
@petertseng petertseng dismissed their stale review October 3, 2019 15:56

the requests are done

@yawpitch
Copy link
Copy Markdown
Contributor

yawpitch commented Oct 6, 2019

I'm going to strongly suggest that this change is unnecessary and does nothing but increases the complexity of the solution without markedly increasing its value as a learning exercise.

As is the student need only consider a binary state for a given place on the board -- does it contain a mine, or no -- and the area outside the playing board needs to merely be ignored. With these changes the student needs to also consider three potential wall tiles, and also whether or not the border itself is properly formed.

As an aesthetic choice it might make sense to display a minesweeper board with a border, but it doesn't make sense to transmit one with that border.

So my vote would be for updating the description.md file to reflect the fact that the border in that file is for display purposes only and has no bearing on the problem.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants