-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Divisors.js #1435
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
Divisors.js #1435
Conversation
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.
The algorithm implementation looks good to me. However, there are few suggestions I would like to provide:
- The function name is
printDivisors()
, but it doesn't prints anything, just returns a list of divisors. I suggest to change it to something likegetDivisors()
. - Change variable name into something meaningful .
- Also add documentations comment.
Please merge PR #1435 if it's fine.
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.
Great Work! Keep it up!
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.
Missing tests. @return{Vector}
should be @return{[]Integer}
(Integer
is treated as an undefined custom type; we could use number
, but that doesn't convey the same information. Ideally consider @typedef
ing Integer).
I'm also not sure how useful this algorithm is given that we already have prime factorization. (This gives you all divisors of a number, but I think that is almost always less useful than prime factorization.)
I have made the necessary changes in Divisors.js. Please review it.
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.
This example is broken
I have made the necessary changes in Divisors.js. Please review it.
It's still missing tests.
I have added a file for computing all unique divisors of a number in javascript in Maths folder. Please merge my PR.