Skip to content

state-of-tic-tac-toe: unique descriptions for test cases#2019

Merged
ErikSchierboom merged 3 commits intomainfrom
jie-state-of-tic-tac-toe-descriptions
Apr 22, 2022
Merged

state-of-tic-tac-toe: unique descriptions for test cases#2019
ErikSchierboom merged 3 commits intomainfrom
jie-state-of-tic-tac-toe-descriptions

Conversation

@jiegillet
Copy link
Copy Markdown
Contributor

Follow-up to #2012.

It was fairly easy to come up with natural descriptions, except for the draws, so I used numbers for those two cases.

@jiegillet jiegillet added x:action/improve Improve existing functionality/content x:module/practice-exercise Work on Practice Exercises x:size/small Small amount of work labels Apr 20, 2022
{
"uuid": "b42ed767-194c-4364-b36e-efbfb3de8788",
"description": "Draw",
"description": "Draw (1)",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't add any info nor does it explain why the case is there, I'd remove the numbering.

Uniqueness is not guaranteed or required in any way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uniqueness is not guaranteed or required in any way.

I would strongly argue against having duplicate descriptions. Yes, they are (currently) not required, but I'd like them to be (I think almost all canonical data adheres to this).

As for this particular case, maybe have the first description be "Draw" and the second one "Another draw"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with changing the description to "another draw", and I would also push towards having unique descriptions, it really helps with generators, or in case of reimplementation to see which test needs to be changed, etc.

Copy link
Copy Markdown
Contributor

@SaschaMann SaschaMann Apr 21, 2022

Choose a reason for hiding this comment

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

That's redundant. It's obvious that in a test group that contains draws, as stated by the group description, all tests will be a draw or another draw. Unless there's an explicit reason for why a certain case exists, e.g. an edge case of a draw, this is useless info and needlessly clutters up the file when scanning and reading it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(You could make an argument that there's no reason to have two drawn games that don't cover any particular edge cases in the first place but that ship has sailed now.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well yes, a redundant naming convention for a redundant situation. The test case is fine BTW, maybe it will be crucial to a student who had an off-by-one error or something.

I don't really mind either way, I just want something unique so it's easier for people down the line.

{
"uuid": "227a76b2-0fef-4e16-a4bd-8f9d7e4c3b13",
"description": "Draw",
"description": "Draw (2)",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

see above

Comment thread exercises/state-of-tic-tac-toe/canonical-data.json
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.

I feel strongly that diagonals should be referred to as falling and rising rather than first and second, because there is no clear ordering to the two diagonals relative to each other. Those changes are highly suggested.

I don't feel strongly about the rows and columns but I think it'd be clearer to refer to them by their position rather than implying an ordering. But I will understand if the consensus decision is that first, second, and third is better for rows and columns. This is because I understand that there is an unambiguous ordering for rows and columns since the language Exercism uses for its written materials is English, which has a clearly-defined order (top-to-bottom and left-to-right).

Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
Comment thread exercises/state-of-tic-tac-toe/canonical-data.json Outdated
@SaschaMann
Copy link
Copy Markdown
Contributor

I feel strongly that diagonals should be referred to as falling and rising rather than first and second, because there is no clear ordering to the two diagonals relative to each other. Those changes are highly suggested.

The mathematical terms would be main diagonal and antidiagonal fwiw: https://en.wikipedia.org/wiki/Main_diagonal. I'm not sure if rising and falling has any precedence. Though I'd be fine with either.

Co-authored-by: Peter Tseng <petertseng@users.noreply.github.com>
@jiegillet
Copy link
Copy Markdown
Contributor Author

Thank you @petertseng, it really is better this way.

I left falling and rising, because it's more descriptive than the proper mathematical description.

@ErikSchierboom ErikSchierboom merged commit dca364c into main Apr 22, 2022
@ErikSchierboom ErikSchierboom deleted the jie-state-of-tic-tac-toe-descriptions branch April 22, 2022 09:02
@ErikSchierboom
Copy link
Copy Markdown
Member

Thanks!

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

Labels

x:action/improve Improve existing functionality/content x:module/practice-exercise Work on Practice Exercises x:size/small Small amount of work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants