Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
ZainabMalik5 wants to merge 8 commits into TheAlgorithms:master
base: master
Choose a base branch
Loading
from ZainabMalik5:new-branch
Open

Conversation

Copy link

@ZainabMalik5 ZainabMalik5 commented Oct 4, 2023

I have added a file for computing all unique divisors of a number in javascript in Maths folder. Please merge my PR.

Copy link
Contributor

@gaurovgiri gaurovgiri left a 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:

  1. The function name is printDivisors(), but it doesn't prints anything, just returns a list of divisors. I suggest to change it to something like getDivisors().
  2. Change variable name into something meaningful .
  3. Also add documentations comment.

Copy link
Author

ZainabMalik5 commented Oct 7, 2023 via email

I have made the necessary changes and sent you a pull request. Please merge it.
...
On Thu, Oct 5, 2023 at 1:43 PM Gaurav Giri ***@***.***> wrote: ***@***.**** requested changes on this pull request. The algorithm implementation looks good to me. However, there are few suggestions I would like to provide: 1. The function name is printDivisors(), but it doesn't prints anything, just returns a list of divisors. I suggest to change it to something like getDivisors(). 2. Change variable name into something meaningful . 3. Also add documentations comment. — Reply to this email directly, view it on GitHub <#1435 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWWATJMLHEVSVWCTYGMGHFLX5ZT3NAVCNFSM6AAAAAA5TCI7SGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNJZGE3TQNBTHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

@appgurueu appgurueu mentioned this pull request Oct 7, 2023
Copy link
Author

Please merge PR #1435 if it's fine.

Copy link
Contributor

@gaurovgiri gaurovgiri left a 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!

Copy link
Collaborator

@appgurueu appgurueu left a comment
edited
Loading

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 @typedefing 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.)

Copy link
Author

I have made the necessary changes in Divisors.js. Please review it.

* @description Generate all unique divisors of a number.
* @param {Integer} num
* @return{[]Integer} divisors
* @example{10-1,2,5,10}
Copy link
Collaborator

@appgurueu appgurueu Oct 9, 2023

Choose a reason for hiding this comment

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

This example is broken

Copy link
Collaborator

I have made the necessary changes in Divisors.js. Please review it.

It's still missing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@appgurueu appgurueu appgurueu requested changes

@raklaptudirm raklaptudirm Awaiting requested review from raklaptudirm raklaptudirm is a code owner

+1 more reviewer

@gaurovgiri gaurovgiri gaurovgiri approved these changes

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /