added check for [bins] argument in fft method of P5, along with unit Test( in es6 style)

[endurance21]

issue :

the bins arguments is very vulnerable , as there is no check to what user provides as the input to bins argument , suppose the user enters a negative value of bins by mistake , then it's a crash !
so there must be a check to what user gives input as "bins" arguments"

solution :

• added a filter (safeBins) to what user enters as argument .
  

• checks are esablished as shown
  

• added a unit test for the function
  

[endurance21]

@therewasaguy
please review this Pr!

[therewasaguy]

I suggest testing the public methods, and making this a private method.

Do you think the functionality of safeBins could be incorporated into safeBufferSize, rather than making it a separate method?

Also, just a note:

In the Web Audio API, the AnalyserNode.frequencyBinCount is Read only (I'm looking at the MDN documentation for reference https://developer.mozilla.org/en-US/docs/Web/API/AnalyserNode). It is always half of the fftSize. So we're kind of doing things backwards in p5.Sound by allowing users to set the binSize directly. We're doing that because it seemed like it would be more intuitive to set a value that represents the array size you get from the analysis, rather than a value that is twice that amount... but I'm having second thoughts about that now.

[therewasaguy]

do we need to add this to the p5 prototype, or can this be a private method that is only used internally?

If it's only added to the prototype for the purpose of writing a test, I think the test should instead be on the public methods that utilize this function behind the scenes.

[endurance21]

@therewasaguy the main problem i face was using AMD here , I was unable to use safeBins defined in src/helper.js in test/tests/helpers.test.js , so I decided to put this method in prototype object of p5 ,
and I am realising now , that it is not the good way of doing it !,
so I will add tests on public function that will wrap this function ! and remove it from p5.prototype

[endurance21]

Also, just a note:

In the Web Audio API, the AnalyserNode.frequencyBinCount is Read only (I'm looking at the MDN documentation for reference https://developer.mozilla.org/en-US/docs/Web/API/AnalyserNode). It is always half of the fftSize. So we're kind of doing things backwards in p5.Sound by allowing users to set the binSize directly. We're doing that because it seemed like it would be more intuitive to set a value that represents the array size you get from the analysis, rather than a value that is twice that amount... but I'm having second thoughts about that now.

yeah ! you are right ! but it is very intuitive and generous to provide user the flexibilty to choose length of array in which analysed data will be served to them !, and we are no where assigning the value to "AnalyserNode.frequencyBinCount" that means it is good to use ,
infact we are creating a level of abstraction that provides ease to user !, so we don't need to remove that ,however we can rename it , name other than bins because it seem to represent "AnalyserNode.frequencyBinCount"
how about name "length" which represents the length of array !

[endurance21]

I suggest testing the public methods, and making this a private method.

i was actually thinking the same , means i need to create a wrapper for this method ? or you have any other suggestions?

[endurance21]

Do you think the functionality of safeBins could be incorporated into safeBufferSize, rather than making it a separate method?

safeBufferSize have other checks than safeBIns has

Also in ,
#419 (comment)
i have agreed to the w3 specs and not giving our user the ability to change the bufferSIZe() and hence we don't need to add tests for it like in safeBins !! so after your confirmation I will close that PR !(#419)

[endurance21]

i would like to draft this one until the module system work is done !