Added permutations and combinations functions (with tests), addressing #99 #128
Conversation
Added permutations and combinations functions (with tests), addressing #99
|
Nice work, thanks Daniel! |
|
I was wondering, shouldn't |
|
Yes, you are correct, it should. I'm going to grab a cup of coffee and then
|
|
Ok great. I just did a commit with some docs added. I would also not mind renaming |
|
One other thought: It would be cool to have BigNumber support for these functions too. Should be easy: |
|
I've changed x to n, and added the k > n error, but I'm not going to submit a new PR yet. I'm going to add BigNumber support. It makes sense because computing the factorial of a number much, much smaller than the upper limit of an integer - like 5 or 6 orders of magnitude smaller - will produce a number much too large to be represented by a 32-bit integer. That said, the factorial function gets extremely slow with large input. Perhaps a medium-term goal could be to add memoization to the factorial function? |
| if (!isInteger(x) || x < 0) { | ||
| throw new TypeError('Positive integer value expected in function combinations'); | ||
| } | ||
| return parseInt(math.factorial(x) / (math.factorial(k) * math.factorial(x-k))); |
There was a problem hiding this comment.
i think, instead of factorial calculation, it is better to calculate multiplication:
x * (x - 1) * (x - 2) * (x - 3) * .... * (x - k + 1) atleast
also, what is a purpose of parseInt here?
There was a problem hiding this comment.
parseInt is the most convenient way of assuring that the JS runtime won't convert the result of the division operation to a floating point value. In spite of the analytical guarantee that the final value will be integral, floating point operations might induce an error. I added it because I got a value of something like 15.00000001, which is not equal to 15.
There was a problem hiding this comment.
@daniel-levin , so you should use "Math.floor"
There was a problem hiding this comment.
Yes, that is true. I've made the relevant changes in my own branch.
The title says it all. If I have omitted anything, or done anything incorrectly, please let me know