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

refact factorize using array reverse and use already existent code #385

Closed
wants to merge 2 commits into from

Conversation

klarkc
Copy link

@klarkc klarkc commented Sep 1, 2021

Might be related #208

@klarkc klarkc changed the title refact factorize using array reverse and already existent code refact factorize using array reverse and use already existent code Sep 1, 2021
@milesfrain
Copy link
Member

Thanks for the contribution.

Unfortunately, this implementation is not entirely correct, since it does not find repeated prime factors (e.g. an input of 18 returns [2, 3] instead of [2, 3, 3]), but the original test cases were not complete enough to capture these corner cases.

I updated the tests in #387 to check these corner cases, and also made it not care about array ordering, so you don't need to include those reverse calls anymore. Feel free to re-apply your rewrite to the latest version to see the failures for yourself.

I don't know if there's a good way to write a valid implementation of this function without pattern matching, so I'd like to prioritize #340 instead of a non-idomatic rewrite of this function. (Note that using pipe guards | will make this solution even cleaner).

@Zelenya
Copy link

Zelenya commented Jun 21, 2023

This can't be merged in the current state. I'm closing things because I want to move stuff around.

@Zelenya Zelenya closed this Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants