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

refactor(curriculum): updating step 22 of stats calculator project #54312

Merged
merged 8 commits into from
Apr 18, 2024

Conversation

jdwilkin4
Copy link
Contributor

Summary of changes

This step comes up a lot on the forum. The issues seem to be the strictness of the tests and the understanding of the problem. The original step had a lot of the answer baked into the hints. But based on forum interaction, it looks like people are copying the answer from the hints without understanding what it means. So I moved the answers out of the hints and expanded the description instead. Also, I removed the separate step to return median and just moved that into step 22.
That way are tests just check for the correct return value without a regex test. This will allow for greater flexibility in correct answers from campers.

Checklist:

@jdwilkin4 jdwilkin4 added the new javascript course These are for issues dealing with the new JS curriculum label Apr 5, 2024
@jdwilkin4 jdwilkin4 self-assigned this Apr 5, 2024
@jdwilkin4 jdwilkin4 requested a review from a team as a code owner April 5, 2024 21:05
@github-actions github-actions bot added scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label labels Apr 5, 2024
@jdwilkin4
Copy link
Contributor Author

Not sure why the test for the Build new API container is failing 🤔

@Supravisor
Copy link
Contributor

Hi @jdwilkin4 , may I work on this one? I had similar issues with CI - Node.js / Test italian and portuguese

@jdwilkin4
Copy link
Contributor Author

@Supravisor

Any issues dealing with CI/CD stuff is handled by the dev team

@huyenltnguyen
Copy link
Member

Not sure why the test for the Build new API container is failing

@jdwilkin4 I think it was a Jest hiccup (it sometimes fails to handle a large amount of tests at the same time). I reran the job and CI is green now.

@jdwilkin4 jdwilkin4 added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Apr 8, 2024
@naomi-lgbt naomi-lgbt added status: merge conflict To be applied to PR's that have a merge conflict and need updating and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Apr 8, 2024
@naomi-lgbt
Copy link
Member

Sorry @jdwilkin4 I merged the step 27 PR and turned this into a mess.

@naomi-lgbt naomi-lgbt added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: merge conflict To be applied to PR's that have a merge conflict and need updating labels Apr 8, 2024
@naomi-lgbt
Copy link
Member

Pulled this one down. I'm not sure about these changes - I do worry that step 22 has now become a LOT to parse/understand at once.

@jdwilkin4
Copy link
Contributor Author

@naomi-lgbt

There might be a way to experiment with breaking this up into multiple steps.

I would have to play around with it.
But if broken up differently it would help with the confusion on the forum
https://forum.freecodecamp.org/search?q=step%2022%20%23javascript%20Statistics

Also, we could still test the final result so it allows for more flexibility in answers like this one
https://forum.freecodecamp.org/t/learn-advanced-array-methods-by-building-a-statistics-calculator-step-22/670107/2

@naomi-lgbt
Copy link
Member

Yes, I'd definitely love to address the consistent tripping points on the forum.

But I also struggled with the state of step 22 here, myself 😅

@jdwilkin4 jdwilkin4 added status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Apr 9, 2024
@jdwilkin4 jdwilkin4 added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP labels Apr 12, 2024
@jdwilkin4
Copy link
Contributor Author

jdwilkin4 commented Apr 12, 2024

@naomi-lgbt

I broke up the step, into a few steps.

The first one focuses on how to check if array length is even or not.
The second one focuses on how to find the median for odd length list of numbers
The third one focuses on how to find the median for even length list of numbers
The fourth one asks them to remove the test code

Then the last one asks them to put all they learned together to check if array is even. If so return the median for that, otherwise return the median for odd list. This lets campers write whatever solution they want to.

I removed the requirement for create the median variable and ternary.
The way I see it, if/else should be a viable solution.
As long as they return the right info, then it should be good.

I tested it out with if/else solution, a ternary solution, the original solution, and a solution created different variables for middle numbers with an if/else. They are passing on my end

…a-structures-22/learn-advanced-array-methods-by-building-a-statistics-calculator/661890c4abae9f2a0eddad6b.md

Co-authored-by: Naomi <nhcarrigan@gmail.com>
Copy link
Member

@Ksound22 Ksound22 left a comment

Choose a reason for hiding this comment

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

Well done, @jdwilkin4!

Co-authored-by: Sem Bauke <semboot699@gmail.com>
@jdwilkin4 jdwilkin4 requested a review from Sembauke April 15, 2024 12:03
@Ksound22
Copy link
Member

This has two approvals already. Why is not yet merged?

@Sembauke Sembauke merged commit 3c1353f into freeCodeCamp:main Apr 18, 2024
22 checks passed
@jdwilkin4
Copy link
Contributor Author

@Ksound22

This has two approvals already. Why is not yet merged?

When someone requests changes that need to be made, the person who requested them needs to approve those changes before it can be merged in. Otherwise, merging is blocked for the PR.

That is how the fcc repo is setup

ahmaxed pushed a commit to ahmaxed/freeCodeCamp that referenced this pull request Apr 25, 2024
…reeCodeCamp#54312)

Co-authored-by: Naomi <nhcarrigan@gmail.com>
Co-authored-by: Sem Bauke <semboot699@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new javascript course These are for issues dealing with the new JS curriculum scope: curriculum Lessons, Challenges, Projects and other Curricular Content in curriculum directory. scope: i18n language translation/internationalization. Often combined with language type label status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants