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

Re-achitect grade-school #683

Merged
merged 2 commits into from
Oct 23, 2021
Merged

Re-achitect grade-school #683

merged 2 commits into from
Oct 23, 2021

Conversation

wolf99
Copy link
Contributor

@wolf99 wolf99 commented Aug 23, 2021

The problem specification for grade-school has some issues.
Trying to re-architect the grade-school tests here so as to build a new problem specification based on and with referenced to a working example.
The implementation is relatively unchanged.

gcov tells me the new tests exercise all lines of, so I start from here and see if a problem specification that reflects these new tests is acceptable.
Having said that, there are some TODOs currently left in these tests. I plan to resolve these in later commits, so that planned additional changes are easier to follow.

@wolf99 wolf99 added x:action/fix Fix an issue x:knowledge/intermediate Quite a bit of Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:size/small Small amount of work labels Aug 23, 2021
@wolf99 wolf99 self-assigned this Aug 23, 2021
@wolf99 wolf99 force-pushed the grade-school branch 4 times, most recently from 566a7c9 to 578490c Compare August 23, 2021 20:17
wolf99 added a commit to exercism/problem-specifications that referenced this pull request Sep 13, 2021
Example implementation here: exercism/c#683

Fixes #1824
@wolf99 wolf99 marked this pull request as ready for review October 20, 2021 21:21
@wolf99
Copy link
Contributor Author

wolf99 commented Oct 20, 2021

I don't like relying on the unit under test for the tearDown(), but didn't see a better way.
As the clear_roster() is not part of the exercise, more of a part of how things need to be implemented in C, it is not tested... perhaps we should give away the implementation "for free" and then we at least know that the implementation starts of as correct for the student.
What do you think?

@wolf99 wolf99 requested a review from a team October 20, 2021 21:24
@wolf99 wolf99 added x:size/medium Medium amount of work and removed x:size/small Small amount of work labels Oct 20, 2021
@ryanplusplus
Copy link
Member

I don't like relying on the unit under test for the tearDown(), but didn't see a better way. As the clear_roster() is not part of the exercise, more of a part of how things need to be implemented in C, it is not tested... perhaps we should give away the implementation "for free" and then we at least know that the implementation starts of as correct for the student. What do you think?

Perhaps it would be better to have an init function that is called in setup() instead of having a clear function called in teardown(). It feels odd to me to assume that the component is ready to go straight away but must be cleaned up afterward. Having it be set up prior to use may be more natural for students.

@wolf99
Copy link
Contributor Author

wolf99 commented Oct 21, 2021

/format

@ryanplusplus
Copy link
Member

ryanplusplus commented Oct 21, 2021

I like extracting the state into roster_t and passing it into all of the methods. That makes a lot more sense than having this be a singleton.

@wolf99
Copy link
Contributor Author

wolf99 commented Oct 21, 2021

a part of how things need to be implemented in C

Whoops. This is complete waffle. 😆
There a much more simple interface that I wasn't seeing the wood for the trees of.

CI link check is failing for a link that recently started to time out sporadically - I'll change that link on a separate PR.

@ryanplusplus
Copy link
Member

Whoops. This is complete waffle. laughing There a much more simple interface that I wasn't seeing the wood for the trees of.

I also missed the obvious solution. It's so obvious in retrospect!

@ryanplusplus ryanplusplus merged commit 9719af1 into main Oct 23, 2021
@ryanplusplus ryanplusplus deleted the grade-school branch October 23, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/fix Fix an issue x:knowledge/intermediate Quite a bit of Exercism knowledge required x:module/practice-exercise Work on Practice Exercises x:size/medium Medium amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants