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

Discuss tournament exercise #122

Closed
ijanos opened this Issue May 6, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@ijanos
Member

ijanos commented May 6, 2016

I see the following issues with the tournament exercise:

  • At first I had no idea what the columns supposed to mean | MP | W | D | L | P. OK, I'm kind of ignorant about sports and I made an educated guess that the W is for Win and L is Loss but still it baffled me at first.
  • No mention of a win worth 3 points, a draw 1 and a loss 0. I had to reverse engineer this from the test outputs. Again this could be do to my ignorance towards sports, maybe this is a common scoring system, but I believe exercism puzzles should be self-contained.
  • the file formats are not explained:
    • the input format can contain comments, lines starting with #
    • what is a valid line?
    • should I expect invalid lines?
    • if the input line team1;team2;win does it mean team1 or team2 won?
  • Why should the tally function return the number of parsed lines? What purpose does it serve? Why does it need a return value at all? I think the author intended the use of Result but it is not expressed anywhere and because tests use unwrap the Optional type can also be used.

And another issue, thinking about the "big picture": this exercise requires reading files, parsing them and writing data to files. I think smaller exercises with only reading and only writing files should precede this one.

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney May 6, 2016

Contributor

Thanks! I haven't done this exericse yet, but it sounds like it has some issues. I will dig into it this weekend.

Contributor

IanWhitney commented May 6, 2016

Thanks! I haven't done this exericse yet, but it sounds like it has some issues. I will dig into it this weekend.

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney May 7, 2016

Contributor

Points 1 & 2 could be addressed in the common readme, I think. A change there would propagate to all tracks that implement Tournament, which would be good.

I think the common Readme should also address the format of the inputs, and include examples. I'm equally baffled by it. I can not tell you which team won a game.

A lot of my confusion comes from the common description. It's unclear to me what problem this exercise is trying to get me to solve. Is it:

  • Parsing of text files
  • Scoring Football results
  • Sorting and displaying tabular data

Or some combination of the three?

If the exercise's goal is "Parse text files", then I think the tests of invalid/commented lines are valid. That is exactly the sort of edge case you want to make students think about.

But if not, then invalid lines are an unnecessary wrinkle in this exercise.

Since this exercise is not widely supported, it's pretty easy to do a quick survey of the other implementations and see what they focus on. From there, maybe we can identify some changes to make in our version.

All three are nearly identical.

  • No input/output files. Instead they define the strings inside the test.
  • Does test invalid lines
  • Tally is the main API, but it returns the expected output, not a number of lines

I think if we bring the Rust implementation in line with that, while also improving the common readme (and adding a common test suite), we could improve this exercise.

Contributor

IanWhitney commented May 7, 2016

Points 1 & 2 could be addressed in the common readme, I think. A change there would propagate to all tracks that implement Tournament, which would be good.

I think the common Readme should also address the format of the inputs, and include examples. I'm equally baffled by it. I can not tell you which team won a game.

A lot of my confusion comes from the common description. It's unclear to me what problem this exercise is trying to get me to solve. Is it:

  • Parsing of text files
  • Scoring Football results
  • Sorting and displaying tabular data

Or some combination of the three?

If the exercise's goal is "Parse text files", then I think the tests of invalid/commented lines are valid. That is exactly the sort of edge case you want to make students think about.

But if not, then invalid lines are an unnecessary wrinkle in this exercise.

Since this exercise is not widely supported, it's pretty easy to do a quick survey of the other implementations and see what they focus on. From there, maybe we can identify some changes to make in our version.

All three are nearly identical.

  • No input/output files. Instead they define the strings inside the test.
  • Does test invalid lines
  • Tally is the main API, but it returns the expected output, not a number of lines

I think if we bring the Rust implementation in line with that, while also improving the common readme (and adding a common test suite), we could improve this exercise.

IanWhitney pushed a commit to IanWhitney/x-common that referenced this issue May 17, 2016

Ian Whitney
Update Tournament readme with more details
Based on the conversation here:
exercism/rust#122

This addresses a few problems that @ijanos had

- What do the column headers mean?
- How are points awarded?
- What is the input format?

There's still a need for a default test json file, but that can come in
a different PR.

IanWhitney added a commit to exercism/problem-specifications that referenced this issue May 18, 2016

Update Tournament readme with more details (#254)
* Update Tournament readme with more details

Based on the conversation here:
exercism/rust#122

This addresses a few problems that @ijanos had

- What do the column headers mean?
- How are points awarded?
- What is the input format?

There's still a need for a default test json file, but that can come in
a different PR.

* Update tournament.md

petertseng added a commit to petertseng/exercism-rust that referenced this issue Jul 2, 2016

tournament: Put inputs/expectations inline, not in files
The previous iteration of reading/writing files is a valuable experience
in itself, but dilutes the focus of this exercise. We should consider an
exercise dedicated to that task if necessary, as discussed in #122.

Note that this preserves all behavior of existing test files. In
particular, the expectation that an invalid line is ignored is
preserved. If you wish for this behavior to be changed, you should
discuss that at exercism/problem-specifications#286 before
we make that change here.

The function `tally` now takes only a `&str` (it previously took two
`Path`, one for input and one for output) and now returns only a
`String` (it previously returned a `Result<u32>` where the `Ok`
counted the number of valid lines).

This commit is necessary but not sufficient for the closing of #122.
Points not covered by this commit but requested by #122 include:
* Explain the input format. Covered by
  exercism/problem-specifications#254
* Explain behavior on invalid lines. Covered by
  exercism/problem-specifications#286

petertseng added a commit to petertseng/exercism-rust that referenced this issue Jul 2, 2016

tournament: Put inputs/expectations inline, not in files
The previous iteration of reading/writing files is a valuable experience
in itself, but dilutes the focus of this exercise. We should consider an
exercise dedicated to that task if necessary, as discussed in #122.

Note that this preserves all behavior of existing test files. In
particular, the expectation that an invalid line is ignored is
preserved. If you wish for this behavior to be changed, you should
discuss that at exercism/problem-specifications#286 before
we make that change here.

The function `tally` now takes only a `&str` (it previously took two
`Path`, one for input and one for output) and now returns only a
`String` (it previously returned a `Result<u32>` where the `Ok`
counted the number of valid lines).

This commit is necessary but not sufficient for the closing of #122.
Points not covered by this commit but requested by #122 include:
* Explain the input format. Covered by
  exercism/problem-specifications#254
* Explain behavior on invalid lines. Covered by
  exercism/problem-specifications#286
@petertseng

This comment has been minimized.

Show comment
Hide comment
@petertseng

petertseng Jul 3, 2016

Member

It's my belief that we'll have resolved all points when all three of the below issues are closed:

We may wish to open another issue to discuss the possibility of file I/O.

Member

petertseng commented Jul 3, 2016

It's my belief that we'll have resolved all points when all three of the below issues are closed:

We may wish to open another issue to discuss the possibility of file I/O.

petertseng added a commit to petertseng/exercism-rust that referenced this issue Jul 3, 2016

tournament: Put inputs/expectations inline, not in files
The previous iteration of reading/writing files is a valuable experience
in itself, but dilutes the focus of this exercise. We should consider an
exercise dedicated to that task if necessary, as discussed in #122.

Note that this preserves all behavior of existing test files. In
particular, the expectation that an invalid line is ignored is
preserved. If you wish for this behavior to be changed, you should
discuss that at exercism/problem-specifications#286 before
we make that change here.

The function `tally` now takes only a `&str` (it previously took two
`Path`, one for input and one for output) and now returns only a
`String` (it previously returned a `Result<u32>` where the `Ok`
counted the number of valid lines).

This commit is necessary but not sufficient for the closing of #122.
Points not covered by this commit but requested by #122 include:
* Explain the input format. Covered by
  exercism/problem-specifications#254
* Explain behavior on invalid lines. Covered by
  exercism/problem-specifications#286

petertseng added a commit to petertseng/exercism-rust that referenced this issue Jul 3, 2016

tournament: Put inputs/expectations inline, not in files
The previous iteration of reading/writing files is a valuable experience
in itself, but dilutes the focus of this exercise. We should consider an
exercise dedicated to that task if necessary, as discussed in #122.

Note that this preserves all behavior of existing test files. In
particular, the expectation that an invalid line is ignored is
preserved. If you wish for this behavior to be changed, you should
discuss that at exercism/problem-specifications#286 before
we make that change here.

The function `tally` now takes only a `&str` (it previously took two
`Path`, one for input and one for output) and now returns only a
`String` (it previously returned a `Result<u32>` where the `Ok`
counted the number of valid lines).

This commit is necessary but not sufficient for the closing of #122.
Points not covered by this commit but requested by #122 include:
* Explain the input format. Covered by
  exercism/problem-specifications#254
* Explain behavior on invalid lines. Covered by
  exercism/problem-specifications#286

petertseng added a commit to petertseng/exercism-rust that referenced this issue Jul 3, 2016

tournament: Put inputs/expectations inline, not in files
The previous iteration of reading/writing files is a valuable experience
in itself, but dilutes the focus of this exercise. We should consider an
exercise dedicated to that task if necessary, as discussed in #122.

Note that this preserves all behavior of existing test files. In
particular, the expectation that an invalid line is ignored is
preserved. If you wish for this behavior to be changed, you should
discuss that at exercism/problem-specifications#286 before
we make that change here.

The function `tally` now takes only a `&str` (it previously took two
`Path`, one for input and one for output) and now returns only a
`String` (it previously returned a `Result<u32>` where the `Ok`
counted the number of valid lines).

This commit is necessary but not sufficient for the closing of #122.
Points not covered by this commit but requested by #122 include:
* Explain the input format. Covered by
  exercism/problem-specifications#254
* Explain behavior on invalid lines. Covered by
  exercism/problem-specifications#286

petertseng added a commit to petertseng/exercism-rust that referenced this issue Jul 5, 2016

config: Move tournament -> between grade-school and queen-attack
With the removal of the file I/O portion of this exercise (via the
previous commit and through discussion in #122) now this exercise
becomes easier and can be moved. The list of skills are now roughly:
string parsing, string formatting, custom sorting.

Also HashMap usage seems natural, as you want to store the results of
each team.

Moving it to after grade-school, another problem that may see HashMap in
its solution.

petertseng added a commit to petertseng/exercism-rust that referenced this issue Jul 9, 2016

tournament: Put inputs/expectations inline, not in files
The previous iteration of reading/writing files is a valuable experience
in itself, but dilutes the focus of this exercise. We should consider an
exercise dedicated to that task if necessary, as discussed in #122.

Note that this preserves all behavior of existing test files. In
particular, the expectation that an invalid line is ignored is
preserved. If you wish for this behavior to be changed, you should
discuss that at exercism/problem-specifications#286 before
we make that change here.

The function `tally` now takes only a `&str` (it previously took two
`Path`, one for input and one for output) and now returns only a
`String` (it previously returned a `Result<u32>` where the `Ok`
counted the number of valid lines).

This commit is necessary but not sufficient for the closing of #122.
Points not covered by this commit but requested by #122 include:
* Explain the input format. Covered by
  exercism/problem-specifications#254
* Explain behavior on invalid lines. Covered by
  exercism/problem-specifications#286

petertseng added a commit to petertseng/exercism-rust that referenced this issue Jul 9, 2016

config: Move tournament -> between grade-school and robot-simulator
With the removal of the file I/O portion of this exercise (via the
previous commit and through discussion in #122) now this exercise
becomes easier and can be moved. The list of skills are now roughly:
string parsing, string formatting, custom sorting.

Also HashMap usage seems natural, as you want to store the results of
each team.

Moving it to after grade-school, another problem that may see HashMap in
its solution.
@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Jul 13, 2016

Contributor

I believe we've addressed this. The problem description now addresses these missing pieces, and the Rust implementation no longer uses the files. Thanks for pointing out the problems with this exercise, @ijanos !

Contributor

IanWhitney commented Jul 13, 2016

I believe we've addressed this. The problem description now addresses these missing pieces, and the Rust implementation no longer uses the files. Thanks for pointing out the problems with this exercise, @ijanos !

@IanWhitney IanWhitney closed this Jul 13, 2016

petertseng added a commit to petertseng/exercism-problem-specifications that referenced this issue Jul 20, 2016

tournament: clarify behavior on invalid lines (ignore)
"Only accept" was a bit vague - this commit changes the mesage to say
that invalid line are ignored, and the tournament scored without them.

This is in line with the behavior of all current tracks. If reviewers
wish a change to this behavior, we can discuss that in a separate PR.

(Note that the go track is an exception, but it clearly states it is an
exception in its test suite visible to students)

Closes #286
Closes exercism/rust#122
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment