The Wayback Machine - https://web.archive.org/web/20201211085502/https://github.com/TheAlgorithms/Go/pull/173
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.

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

Refactor fastExponent.go #173

Closed
wants to merge 4 commits into from
Closed

Conversation

@vedantmamgain
Copy link
Contributor

@vedantmamgain vedantmamgain commented Sep 14, 2020

refactor: condensed the function and added comments for the steps

refactor: condensed the function and added comments for the steps
math/fastExponent.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@vedantmamgain vedantmamgain left a comment

Check it now..

@nomisrevol
Copy link
Collaborator

@nomisrevol nomisrevol commented Sep 29, 2020

You're giving us 3 implementations of the same algorithm. It is really good, but it will be better if in the pull request, you can show us the benchmark between these 3 implementations. Or maybe describe pros and cons of each implementation in PR. Thank you.

Copy link
Contributor Author

@vedantmamgain vedantmamgain left a comment

Have added the benchmarking results of all the three types of implementations of the algorithm.

@nomisrevol
Copy link
Collaborator

@nomisrevol nomisrevol commented Sep 30, 2020

LGTM!

@vedantmamgain
Copy link
Contributor Author

@vedantmamgain vedantmamgain commented Sep 30, 2020

Merge? Or is there anything left?

@nomisrevol
Copy link
Collaborator

@nomisrevol nomisrevol commented Oct 1, 2020

What's primalityTest.go? If it is an implementation for another algorithm, please cherry-pick that commit to other branch and create another pull request. Reset the HEAD to commit e0216bb then I will merge it. Sorry for the late response.

@vedantmamgain
Copy link
Contributor Author

@vedantmamgain vedantmamgain commented Oct 1, 2020

Let me make a fresh pr for both of them separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.