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

replacement of es5 functions to es6 class def feat.P5.FFT #502

Merged
merged 2 commits into from
Jul 20, 2020

Conversation

endurance21
Copy link
Collaborator

area covered
src/fft.js

@endurance21 endurance21 requested review from LilaShiba and therewasaguy and removed request for LilaShiba July 15, 2020 01:18
src/fft.js Outdated
@@ -139,7 +141,7 @@ p5.FFT = function (smoothing, bins) {
* @for p5.FFT
* @param {Object} [source] p5.sound object (or web audio API source node)
*/
p5.FFT.prototype.setInput = function (source) {
FFT.prototype.setInput = function (source) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than use .prototype, we can place these methods within the class, similar to the constructor method (which should be at the top), like

class FFT {
...
  setInput(source) {
   ...
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and yes for sure , we can do so , but a Big BUT is that ,
declaring the method this way , significantly helps in memory optimizations !
as we can see here
https://www.thecodeship.com/web-development/methods-within-constructor-vs-prototype-in-javascript/

should we consider those memory optimization techniques ! ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point to bring up and I'm looking forward to hearing more! Do the con's of memory outweigh the pros of having access to vars inside the constructor?

https://stackoverflow.com/questions/4508313/advantages-of-using-prototype-vs-defining-methods-straight-in-the-constructor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These links are from before ES6.

It's true that we don't want to define the method within the constructor.

To clarify: I'm suggesting that we define it next to / after the constructor, as part of the class. This is syntactic sugar for adding the method to the prototype.

class FFT {
  constructor() { ... }
  setInput(source) { ... }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@therewasaguy wow! i just confirmed it , i made a class and tested it .prototype and its object proto and wolla !
i saw , method was present in there in its prototype object !
That was a nice concept ! i

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That syntactical sugar will get you every time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyle1james ohk ohk

@therewasaguy
Copy link
Member

Looks to be off to a good start! Just a small comment

@endurance21 endurance21 requested a review from therewasaguy July 16, 2020 07:45
@endurance21
Copy link
Collaborator Author

@therewasaguy @kyle1james require your review !

Copy link
Collaborator

@LilaShiba LilaShiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only silly comment I have is if we want to use var or let/const to keep with ES6? Perhaps this is a separate issue to flag for later? Awesome job btw and love your quick turn arounds! 😄

@endurance21
Copy link
Collaborator Author

endurance21 commented Jul 18, 2020

@Kyle1james Yes that is for later prs 😇
I think its ready to merge now 🙃

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one comment on another es6 class feature we could use, whether in this or or a follow-up

src/fft.js Show resolved Hide resolved
@endurance21 endurance21 merged commit 82b85a7 into processing:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants