-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
[#764] New practice exercise book-store
#778
Conversation
Thank you for contributing to Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:
Automated comment created by PR Commenter 🤖. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests went from taking 0.1s to 0.06s after I added it
I see a similar difference is speed on my machine. Still, 0.1s for the test suite is very fast anyway :)
Something we could do is encourage people to use to memoization with processes in a hint/extra instructions so they can practice that.
I think this would only make sense if students struggle with making the solution as fast as yours is without Agent. I find it hard to predict right now if that will be the case. We will need to wait and see, and then ask the mentors what students do with this exercise.
If you're interested in making code run fast, we have an example solution to pythagorean-triplet
that takes 10s on my machine to pass the tests. I wonder if it can be improved further? I think it was already improved from 40s.
|
||
@tag :pending | ||
test "Four groups of four are cheaper than two groups each of five and three" do | ||
# Suggested grouping, [[1,2,3,4],[1,2,3,5],[1,2,3,4],[1,2,3,5]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 leaving the comments in seems helpful
"recursion" | ||
], | ||
"practices": [ | ||
"list-comprehensions" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing fits or everything that fits already has 10 exercises, we could also leave this empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we can also leave lists comprehensions and see if people complain :)
Sounds good :)
My solution (a slight adaptation of this one) runs in 0.06 seconds (on the updated tests of this repo, not the ones in the link). Not bad :) |
Co-authored-by: Angelika Tyborska <[email protected]>
IMO good to merge. I'll check up with Tim if we wants to review too or not. |
Last one of the open issues for new exercises.
In my example solution I use
Agent
to memoize the possible discounts, but it is not really necessary (the tests went from taking 0.1s to 0.06s after I added it), so I didn't include it in the requirements.Speaking of,
enum
andrecursion
are full, so I putlist-comprehensions
but it's a stretch. Something we could do is encourage people to use to memoization with processes in a hint/extra instructions so they can practice that.