-
Notifications
You must be signed in to change notification settings - Fork 15.7k
Factory Functions: Lesson tidyup and content clarifications #30071
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
Open
mao-sz
wants to merge
6
commits into
TheOdinProject:main
Choose a base branch
from
mao-sz:factory-fn-tidyup
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
d2f82e1
Ammend comments and code blocks for clarity and flow
mao-sz a202ee4
Use consistent code style across lesson
mao-sz 363581b
Amend wording for ESM note box
mao-sz 2979f4f
Add clarifying section on purpose of IIFE for module pattern
mao-sz b4994f8
Clarify definition of IIFE and example
mao-sz d2d51e0
Fix grammar
mao-sz 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 hidden or 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.
Uh oh!
There was an error while loading. Please reload this page.
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.
Accidentally came by your PR. It could be me but I'm very not charmed by the function naming here. Since this lesson is being worked on, could we consider something else? For the returned function I think
addoraddSecondNumberwould work better and is about as descriptive as it gets.resultingdoes not sound like/is a conventional js name for a function.For the enclosing
makeAddingfunction I'm not sure yet. Some ideas:makeClosureAdder,makeAddFunctionalthough I'm not a fan of using the function word inside the name but it's very descriptive,createAdderThere 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.
Glossed over this but I agree re: the function names. I also think there's no need for the extra variables - the params themselves are variables scoped to their own functions.
How does this look:
Alternatively, after a more thorough read of this section on closures, I think the section could be simplified and perhaps given a second example at the end that's more practical. This adding function is more appropriate for a simple rundown of the mechanics.
I think part of the simplification could be making clear this is no different from any other use of functions. If you want to create a string based off some args, you write a function that returns a string. Want an array based off args? Same thing - write a function that returns an array. These are all reusable. Want a function based off some args? Write a function that returns a function.
I've found this perspective pretty successful in the server for demistifying closures. I feel this may be a little out of this PR's scope, so if you agree with the above, I think it'll be best to handle that in a separate PR.
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.
Actually I'll have a little deeper think about this section and repurpose this PR as more content clarification with a side of tidyup, rather than the reverse.