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.

Sign up for GitHub

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

Jump to bottom

Migrate greatest common denominator algorithm #201

Merged
merged 6 commits into from
Sep 13, 2018
Merged

Migrate greatest common denominator algorithm #201

merged 6 commits into from
Sep 13, 2018

Conversation

alxmjo
Copy link
Collaborator

@alxmjo alxmjo commented Sep 12, 2018 •
edited
Loading

Migrate greatest common denominator algorithm to header file (as described in #163).

@alxmjo
Remove "using namespace std"
1447e6b 
Importing std namespace can lead to namespace pollution. This commit replaces
those namespace directives with the specific commands required, such as
"using std::cin".
@alxmjo
Merge remote-tracking branch 'origin/reorganise' into reorganise
5944e27
@alxmjo
merge upstream
6c4f136
@alxmjo
Migrate greatest common denominator to header file
a81663d 
Refactor greatest common denominator algorithm to use a header file. Add test
cases as .cpp file to test. Also fix an error in greatest common denominator
function that prevented the accurate calculation of negative arguments. Update
README.
@alxmjo alxmjo self-assigned this Sep 12, 2018
@alxmjo
Copy link
Collaborator Author

alxmjo commented Sep 12, 2018 •
edited
Loading

greatest_common_denominator is a pretty long filename. Also considered euclidean_gcd. That way its sister algorithm could be extended_euclidean_gcd. I'm open to input.

Edit: On second look, that other algorithm is doing something different. So maybe greatest_common_denominator.hpp makes the most sense for this one, and the other one can be extended_euclidean.hpp.

@alxmjo
Fix duplicate CMakeList entry
a904ed7 
The unit test file for greatest_common_denominator was included twice. This
fixes that mistake.
@faheel
Copy link
Member

faheel commented Sep 13, 2018

I haven't heard/read about greatest common "denominator" before. "Divisor" is the commonly used term for a number that divides another number. So we should stick to it.

From Wikipedia, other common terms used for GCD are:

[...] greatest common factor (gcf), highest common factor (hcf), greatest common measure (gcm), or highest common divisor.

@alxmjo
Copy link
Collaborator Author

alxmjo commented Sep 13, 2018

Works for me, and shorter too. Will update and commit right now.

@faheel faheel merged commit 37342f2 into ProAlgos:reorganise Sep 13, 2018
@alxmjo alxmjo deleted the reorganise branch September 14, 2018 13:35
This was referenced Sep 23, 2018
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@faheel faheel faheel approved these changes

Assignees

@alxmjo alxmjo

Labels
Reorganise
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alxmjo @faheel