-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
base: main
Are you sure you want to change the base?
Conversation
Some code blocks split to be easier to follow than trying to follow large comments
Style is more consistent with the rest of the curriculum as well.
Heading added as now required by style guide. Rephrased to link general concept of modules with the current lesson section.
Addresses common confusion with "why not just call the factory function once and omit the IIFE?"
Common confusion that IIFEs are a special construct and not just a func exp called immediately (due to immediate focus on the module pattern). The module pattern doesn't have to be used with factory functions only, just it's one of the more useful use cases.
function makeAdding(firstNumber) { | ||
// "first" is scoped within the makeAdding function | ||
const first = firstNumber; | ||
return function resulting (secondNumber) { | ||
|
||
return function resulting(secondNumber) { | ||
// "second" is scoped within the resulting function | ||
const second = secondNumber; | ||
return first + second; | ||
} | ||
} | ||
// but we've not seen an example of a "function" | ||
// being returned, thus far - how do we use it? | ||
``` | ||
|
||
But we've not seen an example of a "function" being returned thus far - how do we use it? | ||
|
||
```javascript | ||
const add5 = makeAdding(5); | ||
console.log(add5(2)) // logs 7 | ||
console.log(add5(2)); // logs 7 | ||
``` |
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 add
or addSecondNumber
would work better and is about as descriptive as it gets. resulting
does not sound like/is a conventional js name for a function.
For the enclosing makeAdding
function I'm not sure yet. Some ideas: makeClosureAdder
, makeAddFunction
although I'm not a fan of using the function word inside the name but it's very descriptive, createAdder
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.
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:
// firstNumber is scoped to the makeAdder function
function makeAdder(firstNumber) {
// secondNumber is scoped to addSecondNumber
// This function doesn't need to be named, we're just
// making it easier to refer to this function later
return function addSecondNumber(secondNumber) {
return firstNumber + secondNumber;
}
}
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.
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.
That sounds like a solid plan. I think your example code above is an improvement as well. Thanks for doing this Mao.
Because
Due to some inconsistencies with code style and usage of comments, some learners miss some things or get confused in the lesson flow when some parts are in normal text and some parts are in longer comments.
There has also been common confusion regarding the point of IIFEs or what IIFEs actually are (perhaps due to focusing immediately on a practical use of IIFEs without giving a more plain example of what an IIFE really just is - people sometimes overthink it's some special specific construct).
This PR
Issue
Closes #29212
Pull Request Requirements
location of change: brief description of change
format, e.g.Intro to HTML and CSS lesson: Fix link text
Because
section summarizes the reason for this PRThis PR
section has a bullet point list describing the changes in this PRIssue
section