-
-
Notifications
You must be signed in to change notification settings - Fork 783
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
Add mention of swap idiom to 'The Old Switcheroo' exercise #917
Merged
Merged
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
82984b3
Add mention of swap idiom in the 'The Old Switcheroo' exercise
svaberg 6ec504a
Remove redundant brackets in swap exercise
svaberg 78bbd3d
Replace list with tuple in explanation. Add link to glossary for tuple.
svaberg 018ad3e
Append explanation to f2k exercise and add 'local variable' in to the…
svaberg 3f0c41e
Removed the 'Old Switcheroo' challenge
svaberg 64566ef
Update _episodes/08-func.md
svaberg 947f247
Update reference.md
svaberg 1dc0cbc
Merge branch 'gh-pages' into gh-pages
svaberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Based on the resolution of the issue #460, I am not sure we want to include this idiom. Also, with the challenge ultimately being about variable scope rather than how to swap variable values, it may be distracting.
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 was thinking about this overnight. From a distractions perspective the whole swapping functionality of the function may be distracting from the challenge itself. Maybe the The Old Switcheroo exercise should be replaced by a function that does not switch variables at all.
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 agree with you about the swapping being distracting; this seems something of a trick question. Looking at the other exercises based on your comment, I notice the challenge titled "Variables Inside and Outside Functions" seems to highlight the same concept of variable scope. Perhaps the Old Switcheroo could go away altogether in that case. Additionally, scanning over the episode material, I am not seeing us actually introduce the concept of variable scope or local variable...if that is the case, perhaps it is something we need to add... @maxim-belkin ?
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 agree that the Variables Inside and Outside Functions challenge teaches essentially the same content, and in a less cluttered way. As such, maybe The Old Switcheroo should indeed be removed.
Maybe some of the explanatory text from the The Old Switcheroo solution about local variables could be shifted over to the Variables Inside and Outside Functions?
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 think it makes sense to move some of the description, such as adding to the end of what is there:
When the
f2k
function is called, it creates a local variablek
. The function does not return any values and does not alterk
outside of its local copy. Therefore the original value ofk
remains unchanged.I think it could also be useful to create a glossary entry for local variable and link to it there. The text could also differ somewhat if you think it could be explained better in a different way.
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.
Agreed. Does this mean we are moving towards removing The Old Switcheroo? I'll try and implement the changes you propose.
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.
Unless @maxim-belkin or others in the community speaks up about why we should keep the exercise, it would be great for you to carry out the proposed changes. Thank you, @svaberg!
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've carried out all the proposed changes but I have not yet removed the Old Switcheroo. I'll do that now in a separate commit.