Skip to content
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

Update to JavaScript best practices 5. Functions #8

Closed
yiremorlans opened this issue Aug 24, 2022 · 8 comments · Fixed by #10
Closed

Update to JavaScript best practices 5. Functions #8

yiremorlans opened this issue Aug 24, 2022 · 8 comments · Fixed by #10
Assignees

Comments

@yiremorlans
Copy link

Upon reviewing the fundamentals of JS best practices I came across a mix-up in Functions declarations vs function expressions. The description read "Use function expressions rather than function declaration." I believe the original intention was to use "function declaration over function expressions or arrow expressions". The following examples support that function declaration does not throw errors because they provide better scope, while the function and arrow expressions throw an error because they are being called before they are read.

@bob-fornal
Copy link
Owner

bob-fornal commented Aug 24, 2022

Actually, I am much more in favor of arrow function over functional declaration or expression. I can see that some clarification is needed on this issue.

Hoisting is some thing that has caused MANY issues over the years (I've seen it in my code over and over again). I am in favor of being explicit when writing my code and arrow functions actually provide that. I've never been a fan of using hoisting as a good practice, it can hide underlying issues.

Arrow functions ...

  • handle this more effectively.
  • do not allow for use of the new keyword which can cause confusion.
  • have implicit return for a cleaner design.

I do want to hear your thoughts on my response. I saw the PR and will wait on that one until we come to an agreement here.

@bob-fornal bob-fornal linked a pull request Aug 24, 2022 that will close this issue
@bob-fornal bob-fornal self-assigned this Aug 24, 2022
@bob-fornal
Copy link
Owner

Arrow Function Expression rather than Declaration

I originally said I would update this issue to clarify my stance, but I do want to get input on this one.

@bob-fornal
Copy link
Owner

HERE's an article I reference at times over the differences. There are definitely cases for each of the function types (i.e. Classic Function Declaration within Classes), but in general I avoid USING hoisting and like the advantage of not having to use .bind(this) or other variants of setting up the functions initial scope.

@bob-fornal
Copy link
Owner

I keep coming back to this ...

Maybe it would be better to clarify use-cases for the function declarations within this repository ... the original may be a bit too broad.

  1. Prefer arrow functions in general, avoiding hoisting (this is the whole reason arrow functions, let, and const were introduced) and keeping the code clean and readable. Another benefit comes when there is a need to bind the scope.
  2. Within a Class, function declaration makes more sense (other than when used where scope is important, i.e. Observable Subscription).

@bob-fornal
Copy link
Owner

I'm thinking something like this as a better definition ...

Functions

Declaration Functions within Classes

[Better Practice JS008]
  • Use function declaration within classes as methods.

Why? This pattern is widely used and accepted.

Why? This pattern does not have scope issues.

class Categories {
  constructor() {}

  getData() {
    // do something
  }
}

Arrow Functions within Classes

[Better Practice JS017]
  • Use arrow functions within classes when the method will be used as a callback.

Why? This pattern takes into account this without the need for .bind(this).

Arrow Functions rather than Declaration or Expression

[Better Practice JS018]
  • Arrow functions should be used outside of Classes.

Why? Avoid hoisting issues (this is tied to why arrow funtions, const, and let were added).

Why? Better handling of scope when the function is used.

Why? Cleaner presentation of implicit return.

  • In the following code, the function first works even though the actual function follows this line in the code.
  • The function second logically throws a ReferenceError.
  • The function third also logically throws a ReferenceError and takes advantage of scope improvements.
first(); // works
second(); // throws a ReferenceError
third(); // throws a ReferenceError

function first() {
  // do something
}

const second = function() {
  // do something
}

const third = () => {
  // do something
};

@bob-fornal
Copy link
Owner

Here's an example that can go within the Arrow Functions within Classes above ...

In the following code, since the setTimeout is used, an arrow function would be a better fit.

class Categories {
  constructor() {
    setTimeout(this.init, 1000);
  }

  init = () => {
    // do something
  };
}

@yiremorlans
Copy link
Author

Ahh, clearly I didn't have a comprehensive understanding of function expressions and arrow functions! The article that you linked is super informative, but I think your revised version really helped me solidify my understanding a little bit more than before. I think the revision and those examples are 100%! I will go ahead and cancel the PR, I hope it is no inconvenience as I'm still in my first year and it came from inexperience. Please continue to make updates to the better practices because it is very useful for me and I imagine many others!

@bob-fornal
Copy link
Owner

It sounds like you've had a busy first year.

The whole idea of this repository is for people like you to not only see the patterns, but to start to interact and understand the WHY behind some of what we do. Your comments and PR helped me to see an area I could improve, as well as helping you to improve your craft ... which is exactly the intent.

Thanks for the assist!

@bob-fornal bob-fornal linked a pull request Aug 25, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants