Skip to content

feat: Added Double Factorial Iterative Implementation #187

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

Merged
merged 6 commits into from
Mar 5, 2024

Conversation

SpiderMath
Copy link
Contributor

This pull request aims to add the iterative implementation of double factorial in TypeScript
Here's the entry I aim to add an implementation to: https://the-algorithms.com/algorithm/double-factorial-iterative

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Code-wise fine, but unfortunately not really algorithmically interesting...

@SpiderMath
Copy link
Contributor Author

Code-wise fine, but unfortunately not really algorithmically interesting...

Yeah, that is indeed true. This code does not really have anything particularly interesting going for it. The sole reason I added this is so that the TypeScript implementations are available for more algorithms and it was the first one which I saw which didn't have an implementation.
However, I don't mind if you reject this PR since the code is pretty trivial and boring, and isn't that much of a valuable addition to the repository

@SpiderMath SpiderMath requested a review from appgurueu October 11, 2023 12:14
@SpiderMath
Copy link
Contributor Author

Could you please atleast leave a reply @appgurueu ...?

@SpiderMath
Copy link
Contributor Author

Umm, a response or a reply would be highly appreciated

@appgurueu
Copy link
Contributor

appgurueu commented Mar 3, 2024

Sorry for the late reply. I am pretty indecisive on this, which unfortunately lead to me postponing & forgetting about it. Well, nothing has changed; I still think this is relatively straightforward, about as interesting as the factorial implementation - it may be warranted on these grounds, also considering that it has some applications in combinatorics. I'm fairly neutral on this; I'm not opposed to merging this. @raklaptudirm thoughts?

raklaptudirm
raklaptudirm previously approved these changes Mar 4, 2024
Copy link
Member

@raklaptudirm raklaptudirm left a comment

Choose a reason for hiding this comment

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

The implementation seems to be of good quality, so I approve of its merge.

Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

I had to resolve a merge conflict, so you'll have to reapprove @raklaptudirm

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.00%. Comparing base (26ac72d) to head (ecc6a9b).

❗ Current head ecc6a9b differs from pull request most recent head 4c87609. Consider uploading reports for the commit 4c87609 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   96.98%   97.00%   +0.01%     
==========================================
  Files          97       98       +1     
  Lines        1793     1800       +7     
  Branches      345      346       +1     
==========================================
+ Hits         1739     1746       +7     
  Misses         54       54              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@raklaptudirm raklaptudirm merged commit c5b12db into TheAlgorithms:master Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants