-
Notifications
You must be signed in to change notification settings - Fork 31
Fix functions
notebook errors
#251
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
Conversation
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.
Had a look through the changes, the exercises are still too difficult in my opinion but your work definetly helps. I made some comments about older exercises too where I understand with increased difficulty we can expect more from the user, but still I would reorganise a bit, to avoid too much frustration.
functions.ipynb
Outdated
" for item in args[1:]:\n", | ||
" result += item\n", | ||
"\n", | ||
" return result " |
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.
Most of the solution is already there? remove.
Make it clear that although the type of args can be any, they must all be the same.
Define what it means to sum up two None.
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.
Maybe even leave the (None, None) testcase out, not sure how helpful it is.
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.
At some places the ref. function doesn't use the same names for the input variables as the solution function, change to be consistent.
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.
Exercises in the end are missing docstrings.
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.
Exercise longest_sequence:
Part 1 already tests part 2 as well, I assume this is because of the naming of the exercises? in anycase an n^2 solution doesn't pass part1
I don't like the solution at all. I would show the solution with sorting first, which runs fine and is much better understandable than the O(n) solution, saving a log factor shouldn't matter for them.
If you really want you can have two solutions for part1 and 2 which show a nice improvement by the use of sets and hashing.
For part 2 I get no feedback when run, i could imagine this is again an issue with the naming of the exercise?
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.
Exercise password_validator2:
Solution contains Counter function which is not explained.
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.
Buckets reorginaziation:
Import necessary libraries (pathlib and ascii_lowercase, ascii_uppercase?) in the exercise cells.
Make the reference_solutions self contained. It should be possible to copy paste the ref solution and pass the tests. This also means the input variable names should be consistent
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.
I like ths split of exercise two, feels much more natural now. I still don't like the formatting, either change or explicitly tell them how to do it with a hind.
Also part 2 needs some change
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.
This looks good, thanks
Fix #237